feat: runtime events hardening — trace_id, circuit breaker, metrics, prune CLI - #1053
Conversation
Add first-class trace_id (UUID) and parent_event_id (BIGINT FK) columns to genie_runtime_events for distributed tracing. Includes partial index on trace_id, query support via traceId filter, and promotes traceId from event-router data JSONB to the dedicated column.
Add prune-events subcommand with --older-than and --dry-run options for on-demand cleanup of old runtime events. Document all retention policies in docs/RETENTION.md. Includes integration tests.
Wire getEventMetrics() into the metrics CLI to expose events_emitted, events_failed, last_emit_duration_ms, and circuit_state in both text and JSON output. Also log warnings in runtime-emit handler instead of silently swallowing errors.
resolveTargetTeams() was broadcasting executor events to ALL active teams regardless of whether the erroring agent belonged to them. Test fixtures creating executors in shared PG (e.g. err-agent in protocol-router.test.ts) would flood every live agent session with spurious [executor.error] messages. Now checks event.agentId against team.members and team.leader before routing. Events without an agentId still broadcast to all active teams (existing behavior). Closes #1048
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements data retention policies, including a new db prune-events command and comprehensive documentation. It also introduces distributed tracing support by adding trace_id and parent_event_id columns to the genie_runtime_events table. To enhance system resilience and observability, a circuit breaker mechanism and throughput metrics were added to the event publishing logic. Review feedback identifies a critical issue where the lack of an ON DELETE action on the new self-referencing foreign key will cause pruning to fail. Additionally, improvements are suggested for the circuit breaker's state management logic and the database connection cleanup in the pruning command.
| -- 026_events_trace_id.sql — Add trace_id and parent_event_id for distributed tracing (#859) | ||
|
|
||
| ALTER TABLE genie_runtime_events ADD COLUMN IF NOT EXISTS trace_id UUID; | ||
| ALTER TABLE genie_runtime_events ADD COLUMN IF NOT EXISTS parent_event_id BIGINT REFERENCES genie_runtime_events(id); |
There was a problem hiding this comment.
The self-referencing foreign key on parent_event_id lacks an ON DELETE action. By default, this uses NO ACTION, which will cause the genie db prune-events command (and automatic retention) to fail with a foreign key violation if it attempts to delete a parent event while a newer child event still exists in the table. Given that this table is a high-volume event log with a 14-day retention policy, you should use ON DELETE SET NULL to ensure pruning can proceed without breaking the referential integrity of newer events.
ALTER TABLE genie_runtime_events ADD COLUMN IF NOT EXISTS parent_event_id BIGINT REFERENCES genie_runtime_events(id) ON DELETE SET NULL;| isOpen(): boolean { | ||
| if (this.failures < this.threshold) return false; | ||
| if (Date.now() - this.lastFailure > this.cooldown) { | ||
| this.failures = 0; | ||
| return false; | ||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
The isOpen() method contains a side effect that resets this.failures to 0 when the cooldown period expires. This is problematic because:
- It causes the
stategetter to returnclosedinstead ofhalf-openas soon asisOpen()is called, making the state machine inconsistent. - In high-concurrency scenarios, it allows multiple concurrent requests to proceed immediately after the cooldown (a 'thundering herd'), rather than allowing a single trial request in a true 'half-open' state.
- It prevents the
recordSuccess()method from detecting that it was in a failed state, so the 'circuit breaker closed' warning will never be logged.
You should remove the side effect and let recordSuccess() handle the transition back to the closed state.
isOpen(): boolean {
if (this.failures < this.threshold) return false;
return Date.now() - this.lastFailure <= this.cooldown;
}| try { | ||
| const sql = await getConnection(); | ||
|
|
||
| if (options.dryRun) { | ||
| const rows = await sql` | ||
| SELECT count(*) AS cnt | ||
| FROM genie_runtime_events | ||
| WHERE created_at < now() - make_interval(secs => ${intervalSec}) | ||
| `; | ||
| const count = Number(rows[0].cnt); | ||
| console.log(`Would delete ${count} event${count === 1 ? '' : 's'} older than ${options.olderThan}.`); | ||
| } else { | ||
| const result = await sql` | ||
| DELETE FROM genie_runtime_events | ||
| WHERE created_at < now() - make_interval(secs => ${intervalSec}) | ||
| `; | ||
| const count = Number(result.count); | ||
| console.log(`Deleted ${count} event${count === 1 ? '' : 's'} older than ${options.olderThan}.`); | ||
| } | ||
|
|
||
| await shutdown(); | ||
| } catch (err) { | ||
| const message = err instanceof Error ? err.message : String(err); | ||
| console.error(`Prune failed: ${message}`); | ||
| process.exit(1); | ||
| } |
There was a problem hiding this comment.
The shutdown() call should be moved to a finally block to ensure the database connection pool is always closed, even if a query fails. Note that since process.exit(1) terminates the process immediately, you should replace it with process.exitCode = 1 to allow the finally block to execute before the process exits.
try {
const sql = await getConnection();
if (options.dryRun) {
const rows = await sql`
SELECT count(*) AS cnt
FROM genie_runtime_events
WHERE created_at < now() - make_interval(secs => ${intervalSec})
`;
const count = Number(rows[0].cnt);
console.log(`Would delete ${count} event${count === 1 ? '' : 's'} older than ${options.olderThan}.`);
} else {
const result = await sql`
DELETE FROM genie_runtime_events
WHERE created_at < now() - make_interval(secs => ${intervalSec})
`;
const count = Number(result.count);
console.log(`Deleted ${count} event${count === 1 ? '' : 's'} older than ${options.olderThan}.`);
}
} catch (err) {
const message = err instanceof Error ? err.message : String(err);
console.error(`Prune failed: ${message}`);
process.exitCode = 1;
} finally {
await shutdown();
}![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: dc211ac19d
ℹ️ 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".
| -- 026_events_trace_id.sql — Add trace_id and parent_event_id for distributed tracing (#859) | ||
|
|
||
| ALTER TABLE genie_runtime_events ADD COLUMN IF NOT EXISTS trace_id UUID; | ||
| ALTER TABLE genie_runtime_events ADD COLUMN IF NOT EXISTS parent_event_id BIGINT REFERENCES genie_runtime_events(id); |
There was a problem hiding this comment.
The new parent_event_id reference is a strict self-FK without an ON DELETE action, so age-based cleanup can fail as soon as a newer event references an older parent: DELETE ... WHERE created_at < ... will hit a FK violation on the parent row and abort pruning. That directly affects both automatic retention and genie db prune-events, causing runtime-event cleanup to stop and table growth to continue once parentEventId is used.
Useful? React with 👍 / 👎.
| let eventsEmitted = 0; | ||
| let eventsFailed = 0; | ||
| let lastEmitDuration = 0; |
There was a problem hiding this comment.
getEventMetrics() is backed only by module-level counters (eventsEmitted, eventsFailed, lastEmitDuration), so values reset whenever a new CLI process starts. In practice, genie metrics now runs in a fresh process and therefore reports near-zero/default throughput and circuit state instead of the real system-wide runtime-event activity, making the new metrics misleading for operators.
Useful? React with 👍 / 👎.
1 participant
Summary
Hardens the runtime events system with four improvements from the runtime-events-hardening wish.
trace_id+parent_event_idfirst-class columns togenie_runtime_eventsvia migration026_events_trace_id.sql. Index on trace_id for fast query-by-trace.EventCircuitBreakeraroundpublishRuntimeEvent()— opens after 5 consecutive failures, 30s cooldown, half-open recovery. Fixruntime-emit.tsto log warnings instead of silently swallowing.eventsEmitted,eventsFailed,lastEmitDuration,circuitStateexposed viagetEventMetrics(). Wired intogenie metrics now.genie db prune-eventscommand with--older-thanand--dry-run. Newdocs/RETENTION.md.Closes
Test plan
bun run buildpassesbun test— 2045 tests passgenie db prune-events --dry-runshows countgenie metrics nowshows event metrics