Repository navigation
feat(observability): Phase 3 — ClickHouse analytics tier (4 tables → CH, off-by-default) (#890 Phase 3) - #1991
Merged
Merged
Conversation
Contributor
|
Warning Review limit reached
Next review available in: 15 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (72)
📝 WalkthroughWalkthroughThis PR adds an optional ClickHouse analytics tier with configuration, local infrastructure, buffered writes, monitoring read cutovers, startup and shutdown wiring, GDPR export/erasure support, error resolutions, retention handling, migrations, and comprehensive unit and integration tests. ChangesClickHouse analytics tier
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f0089ded6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Owner
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
packages/lib/src/observability/__tests__/clickhouse-client.test.ts (1)
121-144: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the flag-off + GDPR-first + getClient() interaction.
The test on line 138 verifies shared instances when the flag is ON, but there's no test for the rollback-window scenario: flag OFF,
getGdprClient()constructs the instance first, thengetClient()is called. This is the critical edge case where the sharedinstancecache could causegetClient()to return a client despite the flag being off — potentially violating thedisabled → nullcontract documented inclickhouse-client.ts.🧪 Suggested test
it('given the flag on and configured, getClient() and getGdprClient() should share one instance', () => { const createClient = vi.fn().mockReturnValue(fakeClient); const registry = createClickHouseRegistry(makeDeps(configuredEnv, { createClient })); expect(registry.getClient()).toBe(registry.getGdprClient()); expect(createClient).toHaveBeenCalledTimes(1); }); + + it('given flag OFF but config present, getGdprClient() constructing first should NOT cause getClient() to return a client', () => { + const createClient = vi.fn().mockReturnValue(fakeClient); + const registry = createClickHouseRegistry( + makeDeps({ ...configuredEnv, CLICKHOUSE_ENABLED: undefined }, { createClient }), + ); + + // GDPR accessor constructs the shared instance (rollback window) + expect(registry.getGdprClient()).toBe(fakeClient); + expect(createClient).toHaveBeenCalledTimes(1); + + // getClient() must still respect the disabled flag + expect(registry.getClient()).toBeNull(); + }); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/lib/src/observability/__tests__/clickhouse-client.test.ts` around lines 121 - 144, Add a test covering the flag-off rollback scenario where getGdprClient() is called before getClient(): configure a complete environment with CLICKHOUSE_ENABLED unset, mock createClient, assert getGdprClient() returns the client, then assert getClient() returns null and verify the expected client-creation behavior. Place it alongside the existing shared-instance tests in the ClickHouse registry test suite.packages/lib/src/observability/clickhouse-client.ts (1)
145-153: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
isClickHouseAnalyticsInPlay()duplicates env-reading logic from the registry.This function reads
process.envdirectly instead of using the registry'sgetEnvdep, creating a second source of truth for env var names. If var names change, both the registry'sgetEnv(line 106-113) and this function need updating.♻️ Optional: delegate to the registry's getEnv
-export const isClickHouseAnalyticsInPlay = (): boolean => - resolveClickHouseGdprMode({ - CLICKHOUSE_ENABLED: process.env.CLICKHOUSE_ENABLED, - CLICKHOUSE_URL: process.env.CLICKHOUSE_URL, - CLICKHOUSE_HOST: process.env.CLICKHOUSE_HOST, - CLICKHOUSE_USER: process.env.CLICKHOUSE_USER, - CLICKHOUSE_PASSWORD: process.env.CLICKHOUSE_PASSWORD, - CLICKHOUSE_DATABASE: process.env.CLICKHOUSE_DATABASE, - }).mode !== 'unconfigured'; +export const isClickHouseAnalyticsInPlay = (): boolean => + resolveClickHouseGdprMode( + // Reuse the same env snapshot the registry uses + { + CLICKHOUSE_ENABLED: process.env.CLICKHOUSE_ENABLED, + CLICKHOUSE_URL: process.env.CLICKHOUSE_URL, + CLICKHOUSE_HOST: process.env.CLICKHOUSE_HOST, + CLICKHOUSE_USER: process.env.CLICKHOUSE_USER, + CLICKHOUSE_PASSWORD: process.env.CLICKHOUSE_PASSWORD, + CLICKHOUSE_DATABASE: process.env.CLICKHOUSE_DATABASE, + }, + ).mode !== 'unconfigured';Alternatively, expose a
getGdprMode()method on the registry and callregistry.getGdprMode()here to centralize env access.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/lib/src/observability/clickhouse-client.ts` around lines 145 - 153, Update isClickHouseAnalyticsInPlay to use the ClickHouse registry’s existing getEnv dependency, or expose and call a registry getGdprMode() method, instead of reading process.env directly. Centralize the environment variable names and resolve the mode through the registry so env access has a single source of truth.packages/lib/src/logging/graceful-shutdown.ts (1)
26-38: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBound the shutdown drain
run()still waits onflushLogs()anddrainAnalytics()with no deadline, anddrainAnalytics()can block forever if an insert promise never settles. Add a bounded timeout around the shutdown steps so SIGTERM/SIGINT always reachesexit(0).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/lib/src/logging/graceful-shutdown.ts` around lines 26 - 38, Update the graceful-shutdown `run` function to enforce a finite timeout for both `deps.flushLogs()` and `deps.drainAnalytics()`, using a shared timeout helper or `Promise.race` that rejects when the deadline expires. Preserve the existing per-step error logging, then ensure `deps.exit(0)` executes after timeout or failure so shutdown cannot block indefinitely.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/processor/src/server.ts`:
- Around line 406-422: The shutdown handler can reject before reaching
process.exit(0) if either cleanup step fails. Update shutdown to wrap
drainAnalyticsInserts() and queueManager.shutdown() in separate try/catch
blocks, log each failure, and ensure process.exit(0) executes after both
attempts; preserve the existing shuttingDown guard and signal handling.
In `@apps/web/src/lib/monitoring/monitoring-queries.ts`:
- Around line 433-437: Mask email addresses in the ClickHouse failed-login
metadata before returning it. Update the failedLogins mapping in
getErrorAnalytics to apply the shared maskEmailsInMetadata helper (or define an
equivalent local helper), and ensure the result preserves the PG path’s
Record<string, unknown> | null metadata shape.
In `@docs/security/audit-log-retention-policy.md`:
- Around line 24-29: Clarify the ClickHouse analytics-tier description so it
does not imply TTL enforcement for all four tables. Update the paragraph under
“ClickHouse analytics tier (`#890` Phase 3)” to explicitly identify `error_logs`
as the intentional NO-TTL exception, retained only through GDPR erasure, while
the other three tables use their configured TTLs.
- Around line 35-37: Update the ClickHouse TTL timing note in the retention
policy documentation to state that TTL deletion occurs during background merges
or OPTIMIZE TABLE ... FINAL, rather than at insert time. Clarify that rows older
than the TTL horizon may remain visible until the relevant part is merged, and
revise the backfill implication accordingly.
In `@packages/lib/src/logging/logger.ts`:
- Around line 380-385: Remove SIGINT/SIGTERM listener registration from the
shared logger initialization in logger.ts, including the handlers that invoke
shutdown/process.exit. Keep graceful signal handling in the processor app
entrypoint, such as server.ts, or add an explicit opt-out for embedded services
so only the application-owned handler performs queueManager.shutdown().
---
Nitpick comments:
In `@packages/lib/src/logging/graceful-shutdown.ts`:
- Around line 26-38: Update the graceful-shutdown `run` function to enforce a
finite timeout for both `deps.flushLogs()` and `deps.drainAnalytics()`, using a
shared timeout helper or `Promise.race` that rejects when the deadline expires.
Preserve the existing per-step error logging, then ensure `deps.exit(0)`
executes after timeout or failure so shutdown cannot block indefinitely.
In `@packages/lib/src/observability/__tests__/clickhouse-client.test.ts`:
- Around line 121-144: Add a test covering the flag-off rollback scenario where
getGdprClient() is called before getClient(): configure a complete environment
with CLICKHOUSE_ENABLED unset, mock createClient, assert getGdprClient() returns
the client, then assert getClient() returns null and verify the expected
client-creation behavior. Place it alongside the existing shared-instance tests
in the ClickHouse registry test suite.
In `@packages/lib/src/observability/clickhouse-client.ts`:
- Around line 145-153: Update isClickHouseAnalyticsInPlay to use the ClickHouse
registry’s existing getEnv dependency, or expose and call a registry
getGdprMode() method, instead of reading process.env directly. Centralize the
environment variable names and resolve the mode through the registry so env
access has a single source of truth.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 974c16be-4548-4d1b-9c04-a23bf8faa37a
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (72)
.env.exampleapps/admin/.env.exampleapps/admin/src/__tests__/instrumentation.test.tsapps/admin/src/instrumentation.tsapps/admin/src/lib/__tests__/monitoring-queries-clickhouse-cutover.test.tsapps/admin/src/lib/__tests__/monitoring-queries.parity.integration.test.tsapps/admin/src/lib/monitoring-queries.tsapps/admin/vitest.config.tsapps/processor/.env.exampleapps/processor/src/__tests__/server-clickhouse-wiring.test.tsapps/processor/src/server.tsapps/processor/src/workers/account-erasure-worker.tsapps/web/.env.exampleapps/web/next.config.tsapps/web/src/__tests__/instrumentation.test.tsapps/web/src/app/api/internal/monitoring/ingest/route.tsapps/web/src/instrumentation.tsapps/web/src/lib/monitoring/__tests__/monitoring-queries-clickhouse-cutover.test.tsapps/web/src/lib/monitoring/__tests__/monitoring-queries.parity.integration.test.tsapps/web/src/lib/monitoring/monitoring-queries.tsapps/web/src/lib/repositories/conversation-repository.tsdocker-compose.dev.ymldocker-compose.ymldocs/security/audit-log-retention-policy.mdinfrastructure/__tests__/root-compose.test.tspackages/db/drizzle/0194_watery_rawhide_kid.sqlpackages/db/drizzle/meta/0194_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/src/__tests__/deferred-drop-analytics-tables.test.tspackages/db/src/schema/monitoring.tspackages/lib/package.jsonpackages/lib/src/compliance/erasure/__tests__/erasure-plan.test.tspackages/lib/src/compliance/erasure/__tests__/run-erasure.test.tspackages/lib/src/compliance/erasure/erasure-plan.tspackages/lib/src/compliance/export/gdpr-export.test.tspackages/lib/src/compliance/export/gdpr-export.tspackages/lib/src/compliance/retention/monitoring-retention.test.tspackages/lib/src/compliance/retention/monitoring-retention.tspackages/lib/src/config/__tests__/env-validation.test.tspackages/lib/src/config/env-validation.tspackages/lib/src/logging/__tests__/graceful-shutdown.test.tspackages/lib/src/logging/__tests__/logger-database-clickhouse-cutover.test.tspackages/lib/src/logging/__tests__/logger.test.tspackages/lib/src/logging/__tests__/monitoring-purge.test.tspackages/lib/src/logging/graceful-shutdown.tspackages/lib/src/logging/logger-database.tspackages/lib/src/logging/logger.tspackages/lib/src/logging/monitoring-purge.tspackages/lib/src/observability/__tests__/analytics-gdpr.integration.test.tspackages/lib/src/observability/__tests__/analytics-gdpr.test.tspackages/lib/src/observability/__tests__/analytics-inserts.test.tspackages/lib/src/observability/__tests__/analytics-read-core.test.tspackages/lib/src/observability/__tests__/analytics-rows.test.tspackages/lib/src/observability/__tests__/clickhouse-buffer.test.tspackages/lib/src/observability/__tests__/clickhouse-client.test.tspackages/lib/src/observability/__tests__/clickhouse-ddl.test.tspackages/lib/src/observability/__tests__/clickhouse-env.test.tspackages/lib/src/observability/__tests__/clickhouse-integration.test.tspackages/lib/src/observability/__tests__/error-resolutions.integration.test.tspackages/lib/src/observability/__tests__/error-resolutions.test.tspackages/lib/src/observability/analytics-gdpr.tspackages/lib/src/observability/analytics-inserts.tspackages/lib/src/observability/analytics-read-core.tspackages/lib/src/observability/analytics-reads.tspackages/lib/src/observability/analytics-rows.tspackages/lib/src/observability/clickhouse-buffer.tspackages/lib/src/observability/clickhouse-client.tspackages/lib/src/observability/clickhouse-ddl.tspackages/lib/src/observability/clickhouse-env.tspackages/lib/src/observability/error-resolutions.tsscripts/clickhouse-migrate.tsscripts/deferred-migrations/0890-phase6-drop-analytics-pg-tables.sql
2witstudios
force-pushed
the
pu/db-decomp-p3
branch
from
July 10, 2026 22:37
ab4a656 to
e71cd5c
Compare
2witstudios
added a commit
that referenced
this pull request
Jul 10, 2026
…rify CH TTL docs (#890 Phase 3) Addresses CodeRabbit review findings on PR #1991: - processor server.ts (Major): wrap drainAnalyticsInserts() and queueManager.shutdown() each in try/catch so a rejection can never skip process.exit(0) (which would hang the process until SIGKILL). Mirrors the per-step guards in packages/lib graceful-shutdown.ts. The drain step was added by the Phase 3 FIX, so this hardens the shutdown this phase touched. - docs/security/audit-log-retention-policy.md (Minor ×2): (1) the CH-tier paragraph said retention for all 4 tables is enforced by table TTLs, which contradicted error_logs' NO-TTL-by-design row — now names the 3 TTL'd tables and calls out error_logs as GDPR-erasure-only; (2) clarified TTL timing — stored rows are removed lazily at background merges (windows are visibility upper bounds), while optimize_on_insert drops already-aged INSERTED rows, which is the actual backfill constraint. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PXHQJ18NqzJmLjeymPTiKu
Owner
Author
|
CI note (not this branch): the red This branch builds and tests clean on its own (full local typecheck 16/16, lib 724, processor 86, instrumentation 4+4, drop-guard 5, live-CH integration 5/5). It's rebased onto latest master and does not touch |
… lazy client, off-by-default env gating (#890 Phase 3 leaf 1) Analytics-tier groundwork for moving apiMetrics/systemLogs/userActivities/ errorLogs out of the main Postgres. Ships dark: with CLICKHOUSE_ENABLED unset (default) nothing changes — the existing PG writes continue. - createClickHouseBuffer(table, opts) pure factory (packages/lib observability/clickhouse-buffer): auto-flush at 500 rows or 1000ms, double-buffer swap so inserts never block an in-flight flush, flush failures drop the batch and log table+message but never row payloads (PII), drain() for SIGTERM/SIGINT. Full TDD surface per design ref. - Three-state env gating (clickhouse-env, mirrors adminDb's mode contract): off → disabled, on+configured → enabled, on+missing/invalid config → misconfigured (client shell throws at init — never silently drop inserts). Only the exact value CLICKHOUSE_ENABLED='true' enables. - Lazy client shell (clickhouse-client) over @clickhouse/client with injectable deps registry; no env reads or connections at import. NOTE: design ref named @clickhouse/client-node, which doesn't exist on npm — @clickhouse/client is the official Node.js client. - CLICKHOUSE_* registered optional in serverEnvSchema; placeholder-only blocks in root/web/admin/processor .env.example (credentials are server-side secrets). - Pinned clickhouse/clickhouse-server:25.3-alpine dev/CI service in root docker-compose (prod is ClickHouse Cloud; tenant/onprem stacks deliberately excluded) + root-compose test coverage. Smoke: SELECT 1 + currentDatabase() verified against the pinned container through the real client module path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Px6xZZdMHDxiME9MnhMps4
…cutover of the 4 analytics tables (#890 Phase 3 leaf 2) MergeTree DDL (design ref: ORDER BY per hot path, LowCardinality on bounded strings, TTL 90d/30d/180d/none, PARTITION BY toYYYYMM) applied via an idempotent scripts/clickhouse-migrate.ts — explicit apply, not a startup bootstrap, so the app runtime credential never needs DDL grants. Pure (row) → void adapters (insertApiMetric/insertSystemLog/ insertUserActivity/insertError) map PG-shaped writes to snake_case CH rows (pure mappers, injected id/now) and buffer via createClickHouseBuffer into client.insert(JSONEachRow); never-throw, payload-free error logging. Cutover gates on isClickHouseEnabled() inside the logger-database writers plus the two direct write sites (monitoring ingest route systemLogs, conversation-repository userActivities). Off (default) = byte-identical PG behavior; on = CH writes, PG writes for the 4 tables retire. aiUsageLogs stays PG (Phase 4 CDC); readers stay PG until leaf 3. Also: clickhouse service added to docker-compose.dev.yml (internal: true blocks host port publishing — leaf-1 escape) and @clickhouse/client stubbed out of web client bundles (node:os at module scope, dead code in browser). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LbHekMp4SUbgXb2gehD2W3
…de ClickHouse + error_resolutions PG mini-table (#890 Phase 3 leaf 3) Flag-gated (CLICKHOUSE_ENABLED) re-point of the admin/web monitoring readers over the 4 moved analytics tables to ClickHouse, with CH-vs-PG parity tests as the acceptance backbone; off-mode PG paths byte-identical. - lib: analytics-read-core (pure CH SQL builders + PG-parity row parsers: UInt64-string coercion, UTC DateTime→Date, category ''→null un-map, toDayOfWeek→PG DOW), analytics-reads (thin server-side client shells, errors propagate), error-resolutions (resolve upsert → main PG, two-step CH+PG merge reader; stores never SQL-joined) - db: error_resolutions mini-table in main PG (migration 0194 only) — the mutable resolved-flag workflow off immutable CH error rows - admin: getSystemHealth/getApiMetrics/getErrorAnalytics gated to CH (active users stay on PG sessions; email masking on both paths); getUserActivity pinned PG-only (activityLogs = Phase 5) - web: those plus getUserActivity (users JOIN → two-step cross-store lookup with ghost-user drop parity) and getPerformanceMetrics - parity integration tests seed both stores identically in a random recent hour-aligned window (CH TTLs drop expired rows at insert — deep-past seeding silently no-ops) and assert normalized equality - admin vitest: @pagespace/db/* catch-all alias (parity test imports) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016nysLv84avY5pKvfrhWjFy
…store export/erasure, deferred PG drop (#890 Phase 3 leaf 4) - CH TTLs verified correct since table creation (api_metrics 90d, system_logs 30d, user_activities 180d, error_logs NONE by design — GDPR erasure mutations are its only eraser); already pinned by clickhouse-ddl.test.ts, no ALTER needed. - Admin-PG chain tables confirmed no-drop BOTH (security_audit_log Art 17(3)(b); siem_delivery_receipts = anchoring evidence); create-ahead-only maintenance already pinned; documented in docs/security/audit-log-retention-policy.md. - GDPR: new observability/analytics-gdpr.ts (pure param-bound builders/parsers + fail-closed shells). Art 15 export unions PG+CH (CH error rows get resolved from error_resolutions); Art 17 erasure (monitoring-purge → admin DSAR + account-erasure-worker) deletes from BOTH stores via lightweight DELETE mutations. Off-mode byte-identical. Live CH round-trip integration test. - Retention: runMonitoringRetentionCleanup no-ops for the 4 tables when CLICKHOUSE_ENABLED (CH TTLs own retention; pre-cutover PG rows freeze until the Phase 6 drop); aiUsageLogs retention untouched. - Drop PREPARED not executed: scripts/deferred-migrations/ 0890-phase6-drop-analytics-pg-tables.sql (RAISE EXCEPTION guard), pinned by packages/db deferred-drop-analytics-tables.test.ts. Phase 6 tasks filed: kws85p45pvjnivoz3uxs83yp (execute drop), f70ay5fo488veub4fak4feqs (pre-cutover history backfill vs age-out). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SA8aS8EUtws7DuP7QQ4dp7
…e ClickHouse tier (#890 Phase 3 FIX) Resolves the 2 HIGH + 3 MED findings from the Phase 3 adversarial review. HIGH — self-service DSR swallowed CH purge failure: purge-monitoring is now fatal when ClickHouse analytics is in play (buildErasurePlan recomputes it from clickHouseInPlay), so a CH outage during a worker-path erasure aborts the run instead of marking it completed with the subject's error rows retained. HIGH — half-configured deploy silently blacked out telemetry: probeClickHouseStartup() in every app composition root (web/admin instrumentation.ts, processor server.ts) crashes on 'misconfigured' (flag on, creds missing) instead of silently dropping every CH row; drainAnalyticsInserts() wired to graceful shutdown so buffered rows flush on SIGTERM/SIGINT (createShutdownHandler, never-hang on drain failure). MED — GDPR flag-rollback fail-open: export/erasure consult ClickHouse whenever it is CONFIGURED (isClickHouseAnalyticsInPlay), not only when the write flag is on. MED — deferred-drop guard was bypassable: the 4 DROPs now live inside the DO block after the RAISE EXCEPTION so a single statement genuinely aborts under psql -f. MED — error_resolutions orphan assessment recorded. Resumed after the prior FIX agent died mid-work (session boundary); all changes verified: lib 719 pass, web/admin instrumentation 5+5, processor wiring 3, deferred-drop guard 5, typecheck clean, db:generate no-drift. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HW7dvAzDgKbfMqejGaf5gz
…RM test (#890 Phase 3) Two escapes from the recovered FIX commit (a6c12ac6a), caught during PR verification: - web/admin instrumentation.test.ts: `let onSpy: ReturnType<typeof vi.spyOn>` resolves the overloaded process.on spy to the too-restrictive MockInstance<(this: unknown, ...args: unknown[]) => unknown>, failing `tsc --noEmit` (turbo had served a stale cached admin typecheck predating the new test file). Annotate as bare `MockInstance` (defaults to Procedure = (...args: any[]) => any) — no `any`, no cast. - processor server.ts: the FIX wrapped the SIGTERM/SIGINT handler as `() => { void shutdown(sig); }`, a fire-and-forget that broke the pre-existing server.test.ts assertion (`await capturedHandler()` no longer awaited the drain → queue-shutdown → exit chain). Return the shutdown() promise instead — Node ignores handler return values at runtime (identical to the original async handler), restoring test awaitability. Verified: monorepo typecheck 16/16, lib 719, web/admin instrumentation 5+5, processor server 83 (incl. SIGTERM) + wiring 3, drop-guard 5, live-CH integration 5/5, lint clean, db:generate no drift. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PXHQJ18NqzJmLjeymPTiKu
…sh→drain→exit (#890 Phase 3) Codex review (P2): the web/admin instrumentation registered a bespoke SIGTERM/SIGINT listener that called drainAnalyticsInserts() but never awaited it and never exited. Registering a signal listener suppresses Node's default termination, so an idle process could keep running until SIGKILL, losing the graceful-drain window the handler was meant to protect. Root cause: the handler's premise ("covers stdout-only processes where the logger's handler never installs") was false. Logger.getInstance() runs at the logger module's top level and its constructor unconditionally calls startFlushTimer(), which registers a COMPLETE flush → drain → exit handler via createShutdownHandler — regardless of LOG_DESTINATION. That handler flushes buffered logs, drains ALL ClickHouse insert buffers (including direct adapter inserts), then exits. It is the single, terminating owner of graceful shutdown. Fix: drop the redundant, never-terminating drain-only listener from both composition roots and instead deterministically initialize the shared logger at boot (`await import('@pagespace/lib/logging/logger')`), so its shutdown handler is installed even for a process that receives a signal before serving its first request. Tests now assert the composition root registers NO bespoke signal listener (shutdown owned by the logger, covered by graceful-shutdown.test.ts). Verified: web/admin instrumentation 4+4, typecheck + lint clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PXHQJ18NqzJmLjeymPTiKu
…over #1992 Master's #1992 (machines rename) claimed migration 0194 (PageType TERMINAL→MACHINE) + 0195 (machineId data migration), colliding with this branch's 0194 error_resolutions migration. Per repo rules (never hand-edit drizzle SQL), dropped the colliding 0194 during rebase and regenerated the error_resolutions table via `bun run db:generate` — now 0196_married_blink, lineage 0195 → 0196. Table definition is byte-identical to the original (error_id PK, resolved bool default true, resolved_at, resolved_by, resolution). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PXHQJ18NqzJmLjeymPTiKu
…rify CH TTL docs (#890 Phase 3) Addresses CodeRabbit review findings on PR #1991: - processor server.ts (Major): wrap drainAnalyticsInserts() and queueManager.shutdown() each in try/catch so a rejection can never skip process.exit(0) (which would hang the process until SIGKILL). Mirrors the per-step guards in packages/lib graceful-shutdown.ts. The drain step was added by the Phase 3 FIX, so this hardens the shutdown this phase touched. - docs/security/audit-log-retention-policy.md (Minor ×2): (1) the CH-tier paragraph said retention for all 4 tables is enforced by table TTLs, which contradicted error_logs' NO-TTL-by-design row — now names the 3 TTL'd tables and calls out error_logs as GDPR-erasure-only; (2) clarified TTL timing — stored rows are removed lazily at background merges (windows are visibility upper bounds), while optimize_on_insert drops already-aged INSERTED rows, which is the actual backfill constraint. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PXHQJ18NqzJmLjeymPTiKu
2witstudios
force-pushed
the
pu/db-decomp-p3
branch
from
July 10, 2026 22:57
e71cd5c to
9243fa5
Compare
This was referenced Jul 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Phase 3 — ClickHouse analytics tier (part of the DB Decomposition epic, #890)
Why
apiMetrics,systemLogs,userActivities,errorLogsare the four highest-write-volume tables in main Postgres — pure append-only telemetry with no FKs and no mutations, contending with application traffic on the same instance. This phase moves them to ClickHouse Cloud (columnar, TTL-native, built for high-cardinality analytics), leaving main PG for application data. The hash-chain audit tables stay in Admin PG (Phase 2);ai_usage_logsstays in main PG (billing joins + cost-reconcile UPDATEs — Phase 4 replicates it to CH via ClickPipes CDC).Ships dark.
CLICKHOUSE_ENABLEDunset ⇒ every PG write path is byte-identical to master. Off-mode identity is pinned by cutover tests at all six write sites, both reader files, and the GDPR/retention paths.The four leaves
c58246eb5→ rebased) —createClickHouseBuffer(table, {insert, …})(500-row / 1000ms auto-flush, double-buffer, drops + logs payload-free on failure) and a lazygetClickHouseClient()at@pagespace/lib/observability/*. Three-state pure env core (disabled/enabled/misconfigured); credentials are server-side secrets, never bundled,.env.exampleplaceholders only.ded25200d) — idempotentCREATE TABLE IF NOT EXISTSDDL (applied out-of-band byscripts/clickhouse-migrate.ts, so app creds never need DDL grants) + pure(row) → voidnever-throw insert adapters. Flag-gated cutover of the four writers inlogger-database.tsplus two direct write sites; off-mode PG paths byte-identical.error_resolutionsmini-table (2b537368b) — admin/web monitoring UIs re-pointed to server-side CH aggregation queries with parity tests vs the old PG queries. CH rows are immutable, so the resolved-flag workflow moves to a newerror_resolutionsmini-table in main PG (migration0194) — the two-step cross-store join pattern Phase 5 reuses.e201e7523) — CH TTLs own analytics retention (api_metrics 90d / system_logs 30d / user_activities 180d / error_logs none by orchestrator ruling — erasure mutations are its only eraser). Art 15 export unions PG+CH; Art 17 erasure deletes from both stores (fail-closed). The four PG tables'DROPis deferred to Phase 6 (no analytics-history backfill — decision task filed).Review outcome (adversarial /aidd-review, 17 findings)
2 HIGH fixed (
295423c30):purge-monitoringwasfatal:false, so a CH outage during a worker-path erasure left a subject's error rows (message/stack/ip) retained forever under acompletedstatus. Made fatal when ClickHouse is in play.probeClickHouseStartup) that crashes at boot onmisconfigured, wired into web/admin instrumentation + processor composition roots, plus drain-on-SIGTERM/SIGINT so buffered rows aren't lost on deploy.3 MED fixed: GDPR export/erasure now gate on config-present (not the write-cutover flag) so a flag rollback can't strand CH rows from Art-15/17; the deferred-drop SQL's
DROPs moved inside theDOblock after theRAISEso the guard actually aborts under plainpsql -f;error_resolutionsorphan path assessed.2 PR-verification escapes fixed (
6f0089ded, this session): aMockInstancetypecheck error in the new instrumentation tests (turbo had cached a stale admin typecheck) and a stale processor SIGTERM test broken by the new fire-and-forget handler shape.Remaining LOW findings + the userActivities-Art15 pre-existing gap are logged on the Phase 3 board as pending tasks; deferred CH-grants and CI-CH-service tasks are filed (CI does not run the docker CH integration suite — a filed task adds a CH service; unit/lint/typecheck are the CI gate and are green).
Verification
monorepo typecheck 16/16 · lib 719 pass · web/admin instrumentation 5+5 · processor server 83 (incl. SIGTERM) + wiring 3 · drop-guard 5 · live-CH integration 5/5 (docker container) · lint clean ·
db:generateno drift (only0194 error_resolutions, committed).⚠ OFF BY DEFAULT
CLICKHOUSE_ENABLEDunset ⇒ PG unchanged, this PR is a no-op in production. Before any prod enable:scripts/clickhouse-migrate.ts(DDL bootstrap).xqlr5brm5lau6ubjw50ebeaa) and CI-CH-service (pso5dy0gguyr10jtbakj0vy5) gate tasks.DROPof the four tables is deferred to Phase 6 — there is no analytics-history backfill (decision taskf70ay5fo488veub4fak4feqs). Pre-cutover PG rows freeze until the drop.Part of #890 (DB Decomposition & Zero-Trust Logging epic, Phase 3). Handoff: SPEC verifies against the Phase 3 task list, then MERGE.
🤖 Generated with Claude Code
Convergence log
ab4a656be— Codex P2 (signal handlers never terminate): the FIX's web/admin instrumentation registered a bespoke SIGTERM/SIGINT drain-only listener that awaited nothing and never exited (suppressing Node's default termination → hang until SIGKILL). Root cause: theLoggersingleton already registers a completeflush → drain → exithandler unconditionally at module load, draining all CH buffers. Dropped the redundant listener; both composition roots now deterministically initialize the shared logger at boot so its terminating handler is always installed. Codex P2 "keep purging frozen PG rows" is the ratified, documented retention-freeze tradeoff (decision taskf70ay5fo488veub4fak4feqs) — replied, no change.#1992(machines PageType TERMINAL→MACHINE rename): resolved a hard migration collision — refactor(machines): rename TERMINAL page type → MACHINE, terminalId FK → machineId #1992 claimed0194+0195, so the error_resolutions migration was regenerated viadb:generateas0196_married_blink(table byte-identical) andbun.lockreconciled to sdk 2.1.0. Full typecheck 16/16, all suites green post-rebase.process.exit(0)always runs (Major); retention-policy doc corrected (error_logs TTL exception + TTL merge-vs-insert timing, Minor ×2). Deferred with filed tasks — webgetErrorAnalyticsfailed-login email masking (parity-mandate + off-mode byte-identity tension; taskiehscxlxt6e3zb9wkei5hegz); logger signal-handler ownership race with the processor (pre-existing on master, cross-cutting; taskjnbi8j2jos02id2yjqnwq0lb). All 5 threads answered with evidence.