Repository navigation
Harden the PostgreSQL timeline backend for production - #1636
Merged
Merged
Conversation
The suite exercised 5 of 12 EventStore methods and 3 of about 19 QueryOptions fields. Everything else - every filter, cluster scoping, owner lookup, payload encoding, and most of Stats - was covered only by each backend's own tests, which is how a backend shipped that ignored UntilSeq and SequenceOrder while its own suite passed. Adds properties for cluster-context scoping, the namespace/kind/name/source/ event-type/exclude-deleted filters, the since-until window, GetChangesForOwner, a full payload round-trip, the Stats counters and seq bounds, and the per-cluster seen set. All three stores pass. The payload property uses an explicit sub-microsecond timestamp rather than one derived from the clock. time.Now() is microsecond-resolution on darwin, so a fixture built from it cannot detect a backend that truncates to microseconds - which is exactly why the PostgreSQL truncation showed up on Linux CI and not on the contributor's machine. Verified by mutation: reintroducing the truncation now fails on macOS too.
MarkResourceSeen ran one INSERT per resource, synchronously, from the informer add handler. On initial sync that is one round trip per resource in the cluster, serialized against the database's latency: tolerable next to a sidecar, tens of seconds against a managed database in another zone. Marks now update the in-memory set as before and queue the durable write for a background batcher, so IsResourceSeen is unaffected and the handler no longer waits on the network. Measured over 5000 resources against a local database: handler time 1.15s -> 1.5ms, total wall clock 1.15s -> 42ms. Clears stay durable before returning. They are rare, and deferring them risks the opposite failure: a process that dies between the clear and the flush keeps a stale mark and never reports that resource as newly seen again. They share the queue with marks, so ordering holds and a queued mark can never land after the clear that supersedes it - which the existing persistence test caught when an earlier version of this change made clears fire-and-forget. A full queue drops the durable write rather than stalling the informer; the in-memory set is already correct and the cost is one re-report after the next restart. Drops are counted and reported per flush, not per event.
Three things an operator has to know before pointing Radar at a database, none of which were written down: an unreachable database is fatal by design rather than a silent fall back to memory, so the database sits on the startup path; recovery after an outage is not automatic, because only auth-shaped disconnects have a background retry and a database failure classifies as network; and sslmode=require and verify-full are both verified to work, so TLS needs no special handling beyond the DSN.
PR Summary by QodoHarden PostgreSQL timeline persistence for production
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
â Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit fec9ac4. Configure here.
A saturated write queue dropped the clear and closed its completion channel, so ClearResourceSeen returned as though the delete had landed while the row was still present. A mark already queued for the same key then flushed on top of it, leaving the resource marked as seen for good: after the next restart a recreated resource hydrates as already-seen and its first appearance is suppressed, silently and permanently. Marks are still dropped under pressure, which is sound - the in-memory set is already correct and the cost is one re-report after a restart - but a clear is not equivalent, so it now waits for room. That cannot deadlock: the writer drains on a ticker regardless of database health, so a full queue is transient unless the store is closing, which the wait also selects on. Found by two independent review bots on the pull request. The new test fails against the previous behaviour. Also rewrites three comments that recounted how the code got here rather than what it does now.
ClearResourceSeen waited twice: once for room in the write queue, then again for the flush, each up to postgresOperationTimeout. It runs on the informer's delete handler, so a database slow enough to saturate the queue could hold that handler for twice as long as the single round trip this path replaced. Both waits now share one deadline, restoring the previous bound. If the clear cannot be queued within it the call returns instead of waiting again, and says so: the in-memory set is already correct, and the durable row is left for the next delete of the same resource, which is the same outcome the earlier synchronous DELETE had when it timed out.
7 tasks done
This was referenced Sep 7, 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
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.
Description
Production hardening for the PostgreSQL timeline backend merged in #1632. Three changes: the shared store contract now covers more than arrival order, the seen-resource write path no longer blocks the informer on database latency, and the operational failure modes are written down.
No behaviour change for the memory or SQLite backends.
Widen the shared store contract
The conformance suite exercised 5 of 12
EventStoremethods and 3 of about 19QueryOptionsfields. Everything else (every filter, cluster scoping, owner lookup, payload encoding, most ofStats) was covered only by each backend's own tests. That is precisely how a backend shipped ignoringUntilSeqandSequenceOrderwhile its own suite stayed green.Added properties: cluster-context scoping, the namespace/kind/name/source/event-type/exclude-deleted filters, the since-until window,
GetChangesForOwnerscoping, a full payload round-trip, theStatscounters and seq bounds, and the per-cluster seen set. All three stores pass.One detail worth keeping: the payload property uses an explicit sub-microsecond timestamp instead of one derived from the clock.
time.Now()is microsecond-resolution on darwin, so a clock-derived fixture cannot detect a backend that truncates to microseconds. That is why the truncation in #1314 failed on Linux CI and passed on the contributor's Mac. Verified by mutation: reintroducing the truncation now fails on macOS as well.Batch the seen-resource writes
MarkResourceSeenran oneINSERTper resource, synchronously, from the informer add handler. During initial sync that is one round trip per resource in the cluster, serialized against database latency. Fine next to a sidecar; tens of seconds against a managed database in another zone.Marks now update the in-memory set as before and queue the durable write for a background batcher.
IsResourceSeenis unaffected. Measured over 5000 resources against a local database:Clears stay durable before returning. They are rare, and deferring them risks the opposite failure: a process that dies between the clear and the flush keeps a stale mark and never reports that resource as newly seen again. They share the queue with marks, so ordering holds and a queued mark cannot land after the clear that supersedes it. An earlier version of this change made clears fire-and-forget, and the existing persistence test caught it.
A full queue drops the durable write rather than stalling the informer. The in-memory set is already correct, so the cost is one re-report after the next restart. Drops are counted and reported per flush rather than per event.
Document the failure modes
Three things an operator needs before pointing Radar at a database, none of which were written down:
network, so it needs the UI retry or a pod restart.sslmode=requireandverify-fullboth work; TLS needs no handling beyond the DSN.How has this been tested?
sslmode=require, andsslmode=verify-fullwith a pinned CA.go test -raceclean oninternal/timeline.TestCursorForceGrantEndToEndininternal/aiflakes under full parallel load, but it does so on unmodifiedmaintoo and passes in isolation; nothing here touches that package.Known gaps, unchanged
Still open and worth tickets rather than scope creep here: roughly 550 lines duplicated between the PostgreSQL and SQLite stores, no background retry for network-classified disconnects, and no coverage for credential rotation, a slow database, migration against a partially-migrated database, or retention over a large backlog.
Type of change
Checklist
Related issues
Follow-up hardening for #1632. No issue is closed by this change.
Note
Medium Risk
Changes the hot-path seen-set durability semantics (marks may be dropped under load) and expands contract tests across all timeline stores; PostgreSQL informer behavior improves but restart could re-report ânewly seenâ resources if the queue overflowed.
Overview
Production hardening for the PostgreSQL timeline backend: seen-resource persistence no longer blocks informers on per-resource DB round trips, the shared
EventStoreconformance suite now exercises filters and scoping, and in-cluster docs spell out PostgreSQL failure modes.Seen-resource writes move from synchronous
INSERT/DELETEon every mark/clear to a background batcher (queued marks, ~1s / 500-key flushes).MarkResourceSeenstill updates memory immediately and only queues durability; a full queue drops durable marks rather than stalling the informer (logged in aggregate).ClearResourceSeenstays blocking until the clear is flushed, with shared ordering so a mark cannot land after a clear for the same key. Shutdown drains the queue.Conformance (
pkg/timeline/storetest) adds properties for cluster-context scoping, query field filters, since/until,GetChangesForOwner, full event payload round-trip (including sub-microsecond timestamps),Stats, and per-cluster seen/clear behaviorâso backends cannot pass while ignoring filters or encoding.Docs clarify no SQLiteâPostgreSQL migration, unreachable PostgreSQL is fatal (no silent in-memory fallback), recovery needs UI retry or pod restart after outage, and TLS via DSN (
require/verify-full).Tests: new PostgreSQL tests for async persistence and clear under queue saturation.
Reviewed by Cursor Bugbot for commit fb6fe2b. Bugbot is set up for automated code reviews on this repo. Configure here.