fix: audit cleanup C - persistence, concurrency & data integrity (#1708)

docs/design/backup.md

@@ -26,7 +26,7 @@ The backup system protects persistent data (persistence DB, agent memory, and co
 | Scheduled | Configurable interval (default: 6h) | Background, non-blocking |
 | Pre-shutdown | `Company.shutdown()` / SIGTERM | Synchronous, skips compression |
 | Post-startup | After config load, before accepting tasks | Snapshot as recovery point |
-| Manual | `POST /api/v1/admin/backups` | On-demand, returns manifest |
+| Manual | `POST /api/v1/admin/backups` | On-demand, returns manifest. **Requires the `Idempotency-Key` header** (RFC-style retry-safe key, max 255 chars); identical keys within 24h return the cached manifest instead of starting a second backup so a 5xx-driven client retry cannot launch concurrent backups and violate the at-most-one-running invariant. Missing or empty header yields HTTP 400. |
 | Pre-migration | Before restore operations | Safety net, automatic |
 
 ## Restore Flow

docs/design/client-simulation.md

@@ -224,6 +224,17 @@ task lifecycle state machine.
 `ContinuousMode` provides event-driven always-on simulation with scheduled
 requirement generation and review triggers.
 
+### Idempotency
+
+`POST /api/v1/simulations/` registers the run via
+`SimulationStore.register_if_absent`, an atomic check-and-insert under the
+store's lock. A redelivered request (JetStream redelivery, HTTP 5xx-driven
+retry, etc.) carrying the same `simulation_id` returns HTTP 409 Conflict
+instead of spawning a second runner that races the first on
+`update_status` and corrupts metrics. Clients that supply their own
+`simulation_id` get retry safety for free; clients that omit it receive a
+fresh UUID per call and never collide.
+
 ---
 
 ## Configuration

docs/design/observability.md

@@ -197,6 +197,13 @@ names. Format: `"<domain>.<noun>.<verb>"` (e.g., `"api.request.started"`).
 
 All MCP handler log calls go through `logger.warning(EVENT, error_type=type(exc).__name__, error=safe_error_description(exc))` on credential-sensitive paths (never `logger.exception(..., error=str(exc))`) to avoid leaking secrets through traceback frame-locals (SEC-1).
 
+**Event stream events (`observability/events/event_stream.py`):**
+
+| Constant | Level | When fired |
+|----------|-------|------------|
+| `EVENT_STREAM_HUB_PUBLISH_FAILED` | WARNING | A subscriber queue rejected the event (full); the publisher continues (best-effort fan-out). |
+| `EVENT_STREAM_HUB_PUBLISH_DEDUPED` | WARNING | An event was rejected as a duplicate within the per-session sliding-window TTL (default 60s). The hub keys dedup on `event.id`; identical ids within the window are dropped so an upstream retry (e.g. webhook handler that catches a transient publish failure and retries) cannot deliver the same event twice. The window is bounded per session (default 1024 entries, evicted on insert) so a noisy session cannot exhaust memory. |
+
 **API entry-point boundary events (`observability/events/api.py`):**
 
 | Constant | Level | When fired |

docs/licensing.md

@@ -109,6 +109,19 @@ This means:
 
 ---
 
+## Third-Party Dependency Licenses
+
+SynthOrg's default install (SQLite-only) carries only permissive licenses (MIT, Apache-2.0, BSD, ISC). The optional `[postgres]` extra adds two LGPL-3.0-or-later components:
+
+- `psycopg[binary]`: PostgreSQL adapter
+- `psycopg-pool`: connection pool for psycopg
+
+These are linked dynamically (separate `pip`-installable packages) and the LGPL anti-circumvention clause is satisfied by the standard `pip` replacement workflow. Operators who redistribute combined binaries that include the `postgres` extra must publish a NOTICE listing the LGPL components and preserve replacement-version flexibility. Operators using SQLite (the default) carry no LGPL obligations.
+
+The retention of LGPL drivers in the optional extra was evaluated explicitly: the [LGPL Postgres Driver Decision](research/lgpl-postgres-driver-decision.md) compares the trade-offs (asyncpg swap, vendor / fork, accept-with-ADR) and documents why dynamic linkage in an opt-in extra is acceptable under BUSL-1.1's narrowed Additional Use Grant.
+
+---
+
 ## Contributor License Agreement (CLA)
 
 We require a [Contributor License Agreement](https://github.com/Aureliolo/synthorg/blob/main/.github/CLA.md) before merging external contributions. The CLA:

docs/reference/lifecycle-sync.md

@@ -25,3 +25,12 @@ For services whose `stop()` drains across `await` boundaries, wrap the drain in
 - `IntegrationsHealthProber`
 - `EscalationNotifySubscriber`
 - `EscalationSweeper`
+- `ProviderHealthProber` (`providers/health_prober.py`)
+- `OrgInflectionMonitor` (`meta/chief_of_staff/monitor.py`)
+- `BackupScheduler` (`backup/scheduler.py`)
+- `PruningService` (`hr/pruning/service.py`)
+- `NgrokAdapter` (`integrations/tunnel/ngrok_adapter.py`): lifecycle lock only; no spawned background task, so the drain timeout / unrestartable flag do not apply.
+
+### In-place runner variant
+
+`ContinuousMode` (`client/continuous.py`) is **not** a background-task service: `start()` runs the simulation loop on the calling coroutine and only returns when `stop()` signals the stop event. The lifecycle lock therefore guards only the `_running` flag transition (acquire briefly at the top of `start()` to check-and-set, release before the loop body, re-acquire in the `finally` to clear the flag). Holding the lock across the full body would deadlock a second concurrent caller: it would queue on the lock until the first finished and then enter an empty state. Document this distinction when adding new in-place runners.

docs/research/lgpl-postgres-driver-decision.md

@@ -0,0 +1,79 @@
+---
+title: "LGPL Postgres Driver Decision"
+issue: 1708
+audit_findings:
+  - "_audit/runs/2026-05-01-225703/findings/119-license-compat.md"
+  - "_audit/runs/2026-05-01-225703/findings/61-migration-parity.md"
+  - "_audit/runs/2026-05-01-225703/findings/127-lifecycle-lock-pattern.md"
+date: 2026-05-02
+---
+
+# LGPL Postgres Driver Decision
+
+**Issue**: #1708 (audit cleanup C: persistence, concurrency & data integrity)
+**Status**: Decided 2026-05-02
+
+## Bottom line
+
+SynthOrg keeps `psycopg[binary]==3.3.3` and `psycopg-pool==3.3.0` (both LGPL-3.0-or-later) inside the optional `[postgres]` extra. The drivers are linked dynamically, the extra is opt-in, and the BUSL-1.1 narrowed Additional Use Grant does not require redistribution under terms incompatible with LGPL. SQLite remains the default backend for new operators.
+
+## Context
+
+The 2026-05-01 codebase audit (agent 119, license compatibility) flagged the optional `postgres` extra as carrying two LGPL-3.0-or-later dependencies and recommended one of: vendor, swap, or accept-with-ADR. Three options were considered.
+
+| Option | What it costs | What it preserves |
+|---|---|---|
+| **Accept-with-ADR** (chosen) | One ADR + a one-line note in `docs/licensing.md` | Existing 50+ Postgres repository implementations, LISTEN/NOTIFY cross-instance notify channel, JSONB query layer, dual-backend conformance suite (#1505 + #1559), psycopg-pool's connection pooling and async semantics |
+| Swap to `asyncpg` (BSD-3-Clause) | 2-3 days of work; rewrite of every file under `src/synthorg/persistence/postgres/`; LISTEN/NOTIFY rewire; revalidation of the conformance suite; new async-cursor / type-codec idioms | Permissive-license footprint |
+| Vendor / fork psycopg | Indefinite maintenance burden; security patches lag upstream; no realistic path because psycopg is single-licensed LGPL upstream | Same as accept-with-ADR with worse long-term ergonomics |
+
+## Why LGPL is acceptable here
+
+1. **Dynamic linking, not static.** Python imports `psycopg` at runtime via the standard ABI. The LGPL-3.0-or-later anti-circumvention clauses (sections 4-6) cover redistribution of "Combined Works"; they require that operators who redistribute a combined binary must allow the LGPL portion to be replaced. Since psycopg is a separate `pip`-installable package, that condition is satisfied by default: operators can pin a different psycopg version, swap the binary wheel, or replace it entirely without touching SynthOrg's code.
+
+2. **Optional extra.** Operators install the postgres extra explicitly (`pip install synthorg[postgres]` or `uv sync --extra postgres`). The default install path (SQLite-only) carries no LGPL dependencies. Operators who object to LGPL distribution simply do not install the extra.
+
+3. **BUSL-1.1 narrowed Additional Use Grant does not conflict.** Our Additional Use Grant restricts production use by competing-use cases and 500+ employee organizations; it does not impose redistribution terms that contradict LGPL. The two licenses operate on orthogonal axes (licensing-the-source vs. distribution-of-binaries-with-replacement-rights). A SynthOrg redistributor must satisfy both: BUSL for SynthOrg's own source, LGPL for the psycopg portion of any combined binary they ship.
+
+4. **Industry precedent.** psycopg2 (older sibling, also LGPL) ships in major commercial-license SaaS frameworks (e.g. Sentry, GitLab CE/EE) without ever triggering compliance complications. The dynamic-linkage interpretation is settled in the Python ecosystem.
+
+## Consequences
+
+- **For operators using the `postgres` extra**: LGPL-3.0-or-later distribution terms apply to the psycopg portion of any combined binary you redistribute. Practically, this means publishing a NOTICE that lists `psycopg` and `psycopg-pool` as LGPL components and offering replacement-version flexibility (the `pip install` workflow already provides this).
+- **For operators using SQLite** (the default): No LGPL components in the dependency graph. No additional obligations.
+- **For SynthOrg upstream**: No code changes; no rewrite of the persistence layer; the dual-backend conformance suite remains the source of truth for SQLite ↔ Postgres parity.
+
+## Audit-finding resolutions
+
+This ADR also closes two stale findings from the same audit run:
+
+### #61: SQLite migration `idx_wfe_definition_revision`
+
+The audit reported SQLite was missing the `20260424185325_add_idx_wfe_definition_revision.sql` migration that exists in `src/synthorg/persistence/postgres/revisions/`.
+
+**Verified false positive**: SQLite's baseline migration (`src/synthorg/persistence/sqlite/revisions/00000000000000_baseline.sql`) already contains the index at lines 498-499:
+
+```sql
+CREATE INDEX `idx_wfe_definition_revision`
+    ON `workflow_executions` (`definition_id`, `definition_revision`);
+```
+
+The same index also lives at `src/synthorg/persistence/sqlite/schema.sql:543`. SQLite's revision history was squashed at some point (per `docs/guides/persistence-migrations.md` §"Squash") and absorbed all prior incremental migrations into the baseline; Postgres was not squashed, so its history retains the original 2026-04-24 file. The two backends are at schema parity. `atlas migrate validate --env sqlite` and `atlas schema diff --env sqlite` both confirm parity. **No new SQL needed.**
+
+### #127: Lifecycle lock false positives
+
+The audit listed two services as missing the canonical lifecycle pattern:
+
+- **`src/synthorg/communication/conflict_resolution/escalation/sweeper.py`**: already compliant. `_lifecycle_lock` at line 80, `_stop_failed` at line 87, drain timeout at line 88, full canonical pattern in `start()` (lines 90-123) and `stop()` (lines 125-215).
+- **`src/synthorg/hr/training/service.py`**: has no `start()` / `stop()` methods. `TrainingService` is a stateless pipeline orchestrator (with idempotency state); the canonical lifecycle pattern does not apply. The audit was misclassifying the service.
+
+The other six services flagged by agent 127 (health_prober, monitor, scheduler, pruning service, ngrok_adapter, continuous mode) **are** non-compliant and are addressed in this PR.
+
+## References
+
+- [LGPL-3.0-or-later text](https://www.gnu.org/licenses/lgpl-3.0.html) §4 (Combined Works), §5 (Combined Libraries)
+- [GNU LGPL FAQ](https://www.gnu.org/licenses/gpl-faq.html#LGPLDistributionsAndLargerWorks) on dynamic linkage interpretation
+- [BUSL-1.1 text](https://github.com/Aureliolo/synthorg/blob/main/LICENSE) and Additional Use Grant
+- [`docs/licensing.md`](../licensing.md): operator-facing licensing summary
+- [`docs/guides/persistence-migrations.md`](../guides/persistence-migrations.md): squash workflow context for #61
+- [`docs/reference/lifecycle-sync.md`](../reference/lifecycle-sync.md): canonical lifecycle pattern referenced for #127

scripts/mock_spec_baseline.txt

@@ -301,28 +301,6 @@ tests/unit/api/controllers/test_approvals_helpers.py:493:22
 tests/unit/api/controllers/test_approvals_helpers.py:494:38
 tests/unit/api/controllers/test_approvals_helpers.py:511:22
 tests/unit/api/controllers/test_approvals_helpers.py:512:38
-tests/unit/api/controllers/test_backup.py:73:14
-tests/unit/api/controllers/test_backup.py:74:16
-tests/unit/api/controllers/test_backup.py:81:12
-tests/unit/api/controllers/test_backup.py:98:32
-tests/unit/api/controllers/test_backup.py:109:32
-tests/unit/api/controllers/test_backup.py:126:31
-tests/unit/api/controllers/test_backup.py:145:29
-tests/unit/api/controllers/test_backup.py:160:29
-tests/unit/api/controllers/test_backup.py:179:32
-tests/unit/api/controllers/test_backup.py:193:32
-tests/unit/api/controllers/test_backup.py:213:38
-tests/unit/api/controllers/test_backup.py:236:38
-tests/unit/api/controllers/test_backup.py:267:38
-tests/unit/api/controllers/test_backup.py:281:38
-tests/unit/api/controllers/test_backup.py:297:38
-tests/unit/api/controllers/test_backup.py:313:38
-tests/unit/api/controllers/test_backup.py:367:19
-tests/unit/api/controllers/test_backup.py:370:25
-tests/unit/api/controllers/test_backup.py:371:24
-tests/unit/api/controllers/test_backup.py:372:32
-tests/unit/api/controllers/test_backup.py:373:29
-tests/unit/api/controllers/test_backup.py:374:34
 tests/unit/api/controllers/test_collaboration.py:357:23
 tests/unit/api/controllers/test_company.py:108:31
 tests/unit/api/controllers/test_coordination.py:77:18
@@ -2838,14 +2816,6 @@ tests/unit/security/timeout/test_scheduler.py:474:32
 tests/unit/security/timeout/test_scheduler.py:497:19
 tests/unit/security/timeout/test_timeout_checker.py:38:18
 tests/unit/security/timeout/test_timeout_checker.py:147:22
-tests/unit/settings/test_backup_subscriber.py:30:16
-tests/unit/settings/test_backup_subscriber.py:32:22
-tests/unit/settings/test_backup_subscriber.py:33:21
-tests/unit/settings/test_backup_subscriber.py:34:27
-tests/unit/settings/test_backup_subscriber.py:36:14
-tests/unit/settings/test_backup_subscriber.py:39:23
-tests/unit/settings/test_backup_subscriber.py:42:17
-tests/unit/settings/test_backup_subscriber.py:51:27
 tests/unit/settings/test_bridge_config_wiring.py:184:40
 tests/unit/settings/test_bridge_config_wiring.py:185:19
 tests/unit/settings/test_bridge_config_wiring.py:191:40

src/synthorg/api/auth/ticket_store.py

@@ -5,6 +5,14 @@
 uses ``time.monotonic()`` for expiry so it is immune to wall-clock
 adjustments.
 
+The mutating methods (``create``, ``validate_and_consume``,
+``cleanup_expired``) hold a ``threading.Lock`` so the store is safe
+under both single-threaded asyncio handlers and Litestar's threadpool
+dispatch.  The lock spans count-and-insert, pop-and-validate, and
+bulk eviction blocks where a thread switch between the read and the
+mutating step would otherwise let a racing caller exceed the per-user
+cap or observe a half-mutated dict.
+
 .. note::
 
    The store is per-process -- if the ASGI server runs multiple
@@ -15,6 +23,7 @@
 
 import math
 import secrets
+import threading
 
 from pydantic import BaseModel, ConfigDict
 
@@ -28,6 +37,7 @@
     API_WS_TICKET_EXPIRED,
     API_WS_TICKET_INVALID,
     API_WS_TICKET_ISSUED,
+    API_WS_TICKET_LIMIT_EXCEEDED,
 )
 
 logger = get_logger(__name__)
@@ -105,6 +115,7 @@ def __init__(
         self._max_pending = max_pending_per_user
         self._clock: Clock = clock if clock is not None else SystemClock()
         self._tickets: dict[str, _TicketEntry] = {}
+        self._lock = threading.Lock()
 
     @property
     def ttl_seconds(self) -> float:
@@ -136,22 +147,30 @@ def create(self, user: AuthenticatedUser) -> str:
         Returns:
             URL-safe random token string.
         """
-        now = self._clock.monotonic()
-        user_pending = sum(
-            1
-            for e in self._tickets.values()
-            if e.user.user_id == user.user_id and now <= e.expires_at
-        )
-        if user_pending >= self._max_pending:
-            msg = f"Ticket limit exceeded for user {user.user_id}"
-            raise TicketLimitExceededError(msg)
-
-        ticket = secrets.token_urlsafe(get_auth_token_bytes())
-        entry = _TicketEntry(
-            user=user,
-            expires_at=self._clock.monotonic() + self._ttl,
-        )
-        self._tickets[ticket] = entry
+        with self._lock:
+            now = self._clock.monotonic()
+            user_pending = sum(
+                1
+                for e in self._tickets.values()
+                if e.user.user_id == user.user_id and now <= e.expires_at
+            )
+            if user_pending >= self._max_pending:
+                logger.warning(
+                    API_WS_TICKET_LIMIT_EXCEEDED,
+                    user_id=user.user_id,
+                    username=user.username,
+                    pending=user_pending,
+                    cap=self._max_pending,
+                )
+                msg = f"Ticket limit exceeded for user {user.user_id}"
+                raise TicketLimitExceededError(msg)
+
+            ticket = secrets.token_urlsafe(get_auth_token_bytes())
+            entry = _TicketEntry(
+                user=user,
+                expires_at=self._clock.monotonic() + self._ttl,
+            )
+            self._tickets[ticket] = entry
         logger.info(
             API_WS_TICKET_ISSUED,
             user_id=user.user_id,
@@ -164,17 +183,17 @@ def validate_and_consume(self, ticket: str) -> AuthenticatedUser | None:
         """Validate and consume a ticket (single-use).
 
         Atomically removes the ticket via ``dict.pop`` before
-        checking expiry.  In the single-threaded asyncio event loop,
-        ``dict.pop`` cannot be interleaved with another coroutine,
-        so concurrent calls on the same ticket are safely serialised.
+        checking expiry, holding ``self._lock`` so concurrent threads
+        racing on the same ticket cannot both succeed.
 
         Args:
             ticket: Raw ticket string from the client.
 
         Returns:
             The bound ``AuthenticatedUser``, or ``None``.
         """
-        entry = self._tickets.pop(ticket, None)
+        with self._lock:
+            entry = self._tickets.pop(ticket, None)
         if entry is None:
             logger.warning(API_WS_TICKET_INVALID, reason="not_found")
             return None
@@ -200,19 +219,23 @@ def cleanup_expired(self) -> int:
 
         Called periodically by a background task to prevent
         unbounded memory growth from tickets that are requested
-        but never consumed.
+        but never consumed.  Holds ``self._lock`` across the
+        snapshot-and-delete pass so a concurrent ``create()`` cannot
+        change the dict size mid-iteration.
 
         Returns:
             Number of entries removed.
         """
-        now = self._clock.monotonic()
-        expired = [k for k, v in self._tickets.items() if now > v.expires_at]
-        for k in expired:
-            self._tickets.pop(k, None)
+        with self._lock:
+            now = self._clock.monotonic()
+            expired = [k for k, v in self._tickets.items() if now > v.expires_at]
+            for k in expired:
+                self._tickets.pop(k, None)
+            remaining = len(self._tickets)
         if expired:
             logger.info(
                 API_WS_TICKET_CLEANUP,
                 removed=len(expired),
-                remaining=len(self._tickets),
+                remaining=remaining,
             )
         return len(expired)