feat(hooks): PostgresPredicateStateBackend (durable backend PR 2/4) by zmanian · Pull Request #3932 · nearai/ironclaw

zmanian · 2026-05-23T12:32:42Z

What

Durable PostgreSQL backend for predicate sliding-window state — PR 2/4 of the durable-predicate-backend split.

Stacks on #3927 (hooks-predicate-backend-trait-widening), which made PredicateStateBackend a public async trait with a DateTime<Utc> clock and added the contract-tests harness. This PR targets that branch, not reborn-integration. Review/merge #3927 first.

Crate placement

New crate ironclaw_hooks_postgres (out-of-crate durable backend). The contract-tests feature on ironclaw_hooks was added in PR1 specifically so durable backends live out of crate and run the shared suite — exactly the way ironclaw_reborn_event_store is a separate per-domain durable crate. This keeps the Postgres dependency surface out of the hook framework itself. Gated behind a postgres feature, mirroring ironclaw_filesystem / ironclaw_reborn_event_store.

Atomic-tx shape

Each record_* is one READ COMMITTED transaction guarded by a transaction-scoped advisory lock on the bucket:

pg_advisory_xact_lock(key) — serialize same-key writers.
DELETE … WHERE key_hash=$1 AND ts < cutoff — trim out-of-window (also frees aged-out dedup ids).
INSERT … ON CONFLICT (key_hash, id) DO NOTHING — dedup-safe insert.
SELECT COUNT(*) / SUM(value) — aggregate (separate statement so it sees the insert; a data-modifying CTE's effects are NOT visible to a SELECT in the same statement — this was a real bug caught by the contract suite).
Per-key cap (drop-oldest beyond MAX_SAMPLES_PER_KEY) + per-scope LRU quota (MAX_KEYS_PER_TENANT), then re-aggregate inside the same tx if a cap eviction fired so the returned count/sum reflects dropped rows.

Why advisory lock, not REPEATABLE READ: under RR, concurrent same-key writers abort with could not serialize access, forcing a caller retry loop. The advisory lock makes the second writer block until the first commits — same correctness, no spurious aborts. Different keys take different locks and run fully concurrently. The per-scope LRU enforcement takes a second advisory lock in the disjoint (int8) space (always after the key lock → consistent ordering, no deadlock) so concurrent new-key inserts in a scope don't under-evict.

Steady-state hot path = 1 round-trip-equivalent within a single tx (lock + trim + insert + aggregate); cap/LRU statements only fire on overflow / new-key, the rare paths.

DB-clock decision

The trait passes now: DateTime<Utc>. The window-comparison basis is the caller's now (cutoff computed host-side), not NOW(). Rationale: makes the backend a drop-in under the deterministic fixed-clock contract harness, and uses the same Utc::now() the in-memory backend already trusts. The load-bearing cross-host property — replay dedup — is enforced by the PRIMARY KEY (key_hash, id) constraint and is exact regardless of clock skew; atomicity is enforced by the advisory lock, also clock-independent. Cross-host window correctness depends on NTP-level clock agreement, the same assumption the rest of the system makes for timestamps. Documented in src/schema.rs.

id-column type (Codex #3635 finding)

The #3635 docs pinned a 64-char blake3 hex id while the schema said uuid. Postgres uuid is a fixed 128-bit type and rejects a 64-char hex digest. The id column is TEXT, not uuid. Documented in the migration and schema.

Hash keys are BYTEA (blake3 of length-prefixed canonical scope+key serialization — injective, no concatenation-collision).

Test plan

Env-gated on IRONCLAW_HOOKS_POSTGRES_URL / DATABASE_URL (skip-passing when absent, the project's no-testcontainers pattern). Both test binaries are schema-isolated (SET search_path via a deadpool post_create hook) so they can run in parallel against one DB.

8/8 contract functions via predicate_backend_contract_test harness (tests/predicate_state_postgres_contract.rs).
Adversarial (tests/predicate_state_postgres_adversarial.rs): two-host write storm (200 concurrent writes, no count desync); cross-host replay counts once (invocation + value); per-key sample cap bounds count under flood; per-scope LRU bounds distinct keys under concurrent two-host insert pressure.
4 hashing unit tests: injective length-prefixed serialization, map-discriminant separation.

All 17 pass against a local Postgres 14. cargo fmt --all, cargo clippy --all --benches --tests --examples --all-features -- -D warnings, and cargo check --features postgres are clean.

Divergence note

The base branch's in-memory backend uses drop-oldest for the per-key cap (no WindowOverflow fail-closed variant). This backend matches that. If a later in-memory bugfix switches to fail-closed, this backend should follow — flagged in the module docs.

🤖 Generated with Claude Code

…tract (durable backend PR 1/4) Widen the predicate sliding-window backend contract from pub(crate) to a stable public surface that out-of-crate durable backends (Postgres, libSQL) can implement. This is PR 1 of the 4-PR durable-backend split; no durable impls land here. Surface changes: - Make `PredicateStateBackend`, `InvocationKey`, `ValueKey`, and `PredicateBackendError` `pub` (were `pub(crate)`). Replace the "intentionally pub(crate)" rustdoc with a stable-contract note. - Convert the trait to `#[async_trait]`. Both target durable backends are async; a sync trait would force `block_on` on the dispatch hot path. `PredicateEvaluator::evaluate`/`evaluate_at` and the sole production caller (`PredicateBackedBeforeCapabilityHook::evaluate`, already async) ripple to `.await` cleanly. - Change the clock from `std::time::Instant` to `chrono::DateTime<Utc>`. `Instant` is not serializable across processes; durable rows need a wall-clock timestamp. The in-memory backend stores `DateTime<Utc>` directly; window trimming stays age-based (`front_ts < now - window`). Contract-test harness scaffold: - Add `predicate_state::contract`, a feature-gated (`contract-tests`) module of `pub async fn` contracts each taking a backend factory closure, plus a `predicate_backend_contract_test!` macro — mirrors `ironclaw_memory::contract_tests` (#3918). The in-memory backend is wired through the suite, proving the harness shape works with one backend before PRs 2/3 drop in durable impls. In-memory backend behavior is unchanged apart from the clock-type adaptation; all prior unit tests are preserved (converted to async + `DateTime<Utc>` fixtures). Coordination with the in-flight #3635 bugfix: - The `Instant::checked_sub` underflow fix is SUPERSEDED here: with `DateTime<Utc>`, subtraction saturates rather than underflowing, so the hazard does not arise. Age-based trimming still holds. - The concurrent `hooks-predicate-state-bugfix` branch/PR is NOT yet on origin. If its cap-eviction overflow fix (e.g. a `WindowOverflow` variant on `PredicateBackendError`) lands first, this PR should be rebased onto it so the now-public error enum carries that variant — do not clobber the security fix. If this lands first, fold the overflow variant into the public enum in that PR. Blast radius confined to ironclaw_hooks + ironclaw_reborn; full workspace builds clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Implements the durable PostgreSQL backend for predicate sliding-window state, satisfying the `PredicateStateBackend` contract widened to a public async trait in PR #3927. New crate `ironclaw_hooks_postgres` (out-of-crate durable backend, the pattern the `contract-tests` feature on `ironclaw_hooks` was designed for; mirrors `ironclaw_reborn_event_store`). Keeps the Postgres dependency surface out of the hook framework itself. Correctness: - Atomic record-and-read: each `record_*` runs in one READ COMMITTED tx guarded by a transaction-scoped advisory lock on the bucket. The advisory lock (not REPEATABLE READ) serializes same-key writers by BLOCKING the second writer rather than aborting it with a serialization failure — no caller retry loop. Trim, insert, cap-evict, aggregate all share the one tx. - Cross-host replay dedup via PRIMARY KEY (key_hash, id) + INSERT ON CONFLICT DO NOTHING — exact regardless of clock skew. - Running-sum consistency under eviction: cap eviction re-aggregates inside the same tx so the returned sum reflects dropped rows. - Per-key sample cap (MAX_SAMPLES_PER_KEY, drop-oldest) and per-scope distinct-key LRU quota (MAX_KEYS_PER_TENANT) match the in-memory backend. Scope-quota enforcement takes a second advisory lock in the disjoint (int8) lock space so concurrent new-key inserts in a scope don't under-evict. Schema: single `hook_predicate_counters` table, BYTEA blake3 hash keys, TEXT id column (NOT uuid — Postgres uuid rejects the 64-char blake3 hex digest; resolves the #3635 docs/schema contradiction in favor of TEXT). Idempotent CREATE ... IF NOT EXISTS applied via run_migrations(); the per-crate pattern, not the legacy main-binary refinery migrations. DB-clock decision: window comparison basis is the caller's `now` (the same Utc::now() the in-memory backend trusts), making this a drop-in under the deterministic-clock contract harness. Replay dedup and atomicity do NOT depend on clock agreement. Tests (env-gated on IRONCLAW_HOOKS_POSTGRES_URL / DATABASE_URL, skip when absent; schema-isolated so the two test binaries can share one DB): - All 8 shared contract functions via the `contract-tests` harness. - Adversarial: two-host write storm (no count desync), cross-host replay (count + value), per-key cap flood, per-scope LRU under concurrent insert pressure across two hosts. - 4 hashing unit tests (injective length-prefixed serialization). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

gemini-code-assist

Code Review

This pull request introduces the ironclaw_hooks_postgres crate, providing a durable PostgreSQL-backed implementation of the PredicateStateBackend trait. The implementation ensures cross-host consistency through transaction-scoped advisory locks and includes comprehensive logic for bucket hashing, sample capping, and LRU eviction. Review feedback identified an improvement opportunity to use u64 for length prefixes in hashing to prevent potential collisions and align with repository best practices. Additionally, a correction was suggested for documentation in schema.rs that incorrectly described the transaction isolation level used in the backend.

gemini-code-assist · 2026-05-23T12:35:58Z

+    // 4-byte length prefix makes the field boundary unambiguous; u32 is
+    // ample for any tenant/capability/field string we will ever see.
+    let len = u32::try_from(field.len()).unwrap_or(u32::MAX);


Using u32 for the length prefix could lead to hash collisions for inputs longer than u32::MAX bytes. While unwrap_or(u32::MAX) prevents a panic, it means that any string longer than u32::MAX will use u32::MAX as its length, which could lead to collisions if their prefixes match.

A general rule for this repository states: 'Prefer using u64 for length prefixes in binary encodings to ensure that conversions from usize are infallible on all supported platforms...'.

Using u64 would be safer and align with the repository's best practices. On 64-bit platforms, usize to u64 is a no-op, and on 32-bit platforms it's a safe widening conversion.

Suggested change

// 4-byte length prefix makes the field boundary unambiguous; u32 is

// ample for any tenant/capability/field string we will ever see.

let len = u32::try_from(field.len()).unwrap_or(u32::MAX);

// 8-byte length prefix makes the field boundary unambiguous.

let len = field.len() as u64;

References

Prefer using u64 for length prefixes in binary encodings to ensure that conversions from usize are infallible on all supported platforms, avoiding panics and potential DoS vectors caused by .expect() on overflow.

gemini-code-assist · 2026-05-23T12:35:58Z

+//! running prune + insert + aggregate inside one `REPEATABLE READ`
+//! transaction, also clock-independent.


This comment incorrectly states that atomicity is enforced using a REPEATABLE READ transaction. The implementation in backend.rs actually uses READ COMMITTED along with transaction-scoped advisory locks.

The implementation's documentation and the PR description correctly explain the reasoning for this choice (to avoid serialization failures and retries). This comment should be updated to be consistent with the implementation to avoid confusion for future maintainers.

Suggested change

//! running prune + insert + aggregate inside one `REPEATABLE READ`

//! transaction, also clock-independent.

// running prune + insert + aggregate inside one READ COMMITTED

// transaction with an advisory lock, also clock-independent.

References

Documentation and comments for complex architectural logic, such as multi-tenant isolation policies, must precisely match the implementation to avoid misleading developers about the security model.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: eb6d1af1cf

ℹ️ 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".

chatgpt-codex-connector · 2026-05-23T12:37:15Z

+                            SELECT key_hash, MAX(ts) AS last_ts
+                              FROM hook_predicate_counters
+                             WHERE scope_hash = $1 AND kind = $2
+                               AND key_hash <> $3
+                             GROUP BY key_hash


Match quota eviction ordering with in-memory backend

The tenant-quota eviction query ranks candidates by MAX(ts) (newest sample) and then evicts the key with the oldest last activity, but the in-memory implementation evicts by the oldest front timestamp (evict_lru_*_for_tenant uses each bucket’s oldest retained entry). This can evict a different key under the same state (for example, a key with one very old and one recent sample), causing backend-dependent counter resets and inconsistent rate-limit behavior after switching implementations. Use oldest-front ordering (e.g., MIN(ts)) to keep parity with the trait’s existing in-memory semantics.

Useful? React with 👍 / 👎.

zmanian · 2026-05-23T13:03:41Z

Superseded by #3933. This PR was auto-closed when its base branch hooks-predicate-backend-trait-widening was deleted on the squash-merge of the trait widening (#3927, now in reborn-integration). #3933 re-homes the Postgres-specific commit onto reborn-integration and aligns the per-key cap to fail-closed (WindowOverflow) to match the now-uniform PredicateStateBackend contract (#3929).

…eplaces #3932) (#3933) * feat(hooks): PostgresPredicateStateBackend (durable backend PR 2/4) Implements the durable PostgreSQL backend for predicate sliding-window state, satisfying the `PredicateStateBackend` contract widened to a public async trait in PR #3927. New crate `ironclaw_hooks_postgres` (out-of-crate durable backend, the pattern the `contract-tests` feature on `ironclaw_hooks` was designed for; mirrors `ironclaw_reborn_event_store`). Keeps the Postgres dependency surface out of the hook framework itself. Correctness: - Atomic record-and-read: each `record_*` runs in one READ COMMITTED tx guarded by a transaction-scoped advisory lock on the bucket. The advisory lock (not REPEATABLE READ) serializes same-key writers by BLOCKING the second writer rather than aborting it with a serialization failure — no caller retry loop. Trim, insert, cap-evict, aggregate all share the one tx. - Cross-host replay dedup via PRIMARY KEY (key_hash, id) + INSERT ON CONFLICT DO NOTHING — exact regardless of clock skew. - Running-sum consistency under eviction: cap eviction re-aggregates inside the same tx so the returned sum reflects dropped rows. - Per-key sample cap (MAX_SAMPLES_PER_KEY, drop-oldest) and per-scope distinct-key LRU quota (MAX_KEYS_PER_TENANT) match the in-memory backend. Scope-quota enforcement takes a second advisory lock in the disjoint (int8) lock space so concurrent new-key inserts in a scope don't under-evict. Schema: single `hook_predicate_counters` table, BYTEA blake3 hash keys, TEXT id column (NOT uuid — Postgres uuid rejects the 64-char blake3 hex digest; resolves the #3635 docs/schema contradiction in favor of TEXT). Idempotent CREATE ... IF NOT EXISTS applied via run_migrations(); the per-crate pattern, not the legacy main-binary refinery migrations. DB-clock decision: window comparison basis is the caller's `now` (the same Utc::now() the in-memory backend trusts), making this a drop-in under the deterministic-clock contract harness. Replay dedup and atomicity do NOT depend on clock agreement. Tests (env-gated on IRONCLAW_HOOKS_POSTGRES_URL / DATABASE_URL, skip when absent; schema-isolated so the two test binaries can share one DB): - All 8 shared contract functions via the `contract-tests` harness. - Adversarial: two-host write storm (no count desync), cross-host replay (count + value), per-key cap flood, per-scope LRU under concurrent insert pressure across two hosts. - 4 hashing unit tests (injective length-prefixed serialization). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(hooks-postgres): take victim-key advisory lock during scope-LRU eviction (deadlock + race) enforce_scope_quota deleted victim buckets' rows while holding only the per-scope advisory lock, never the victims' per-key advisory lock. That broke the per-bucket serialization guarantee in two ways: * Torn aggregate: the LRU pass could delete key B's rows while another transaction was recording B under B's own per-key lock, so the recorder's COUNT/SUM straddled a delete it never serialized against. * Deadlock: a recorder of victim key B holds B's per-key lock and then waits on the scope lock inside its own quota pass, while the LRU transaction holds the scope lock and waits on B's row locks to delete them -> cycle. Fix: evict victims one at a time, each guarded by that victim key's per-key advisory lock acquired with the NON-blocking pg_try_advisory_xact_lock. An in-flight victim (lock already held by a concurrent recorder) is skipped, not waited on, so the deadlock cycle cannot form and eviction never deletes rows out from under an unserialized transaction. Candidates are over-fetched beyond the evict count so skips still meet the per-scope quota. Fail-closed WindowOverflow semantics and the dedup/atomicity invariants are unchanged. Adds a lib unit test pinning the eviction try-lock key derivation equal to the recorder's lock key (lock-acquisition invariant, no DB needed), a deterministic raw-SQL regression test proving the try-lock on an in-flight victim returns immediately rather than blocking, and a concurrency stress test asserting no deadlock and no torn aggregate. The integration tests gate on IRONCLAW_HOOKS_POSTGRES_URL/DATABASE_URL and were verified against a real Postgres 14. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * refactor(hooks-postgres): make migration SQL the single source via include_str! firat (PR #3933 maintainability audit): schema.rs embedded a hand-copied DDL const documented as "byte-compatible" with migrations/V1__predicate_counters.sql, with nothing enforcing equivalence — silent drift risk. Pull the DDL directly from the .sql file via include_str! so there is exactly one copy. batch_execute tolerates the file's leading -- comment block, so no statement splitting is needed. run_migrations is exercised by the contract/adversarial suites, confirming the included SQL applies. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(hooks-postgres): rank LRU eviction victims by MIN(ts), matching in-memory/libSQL serrrfirat (PR #3933): the Postgres scope-LRU victim query ranked keys by MAX(ts) (most-recent activity) while the in-memory backend evicts by each bucket's OLDEST retained sample (entries.front() + min_by_key) and libSQL does the same (oldest-front). The doc comment claimed oldest-front but the SQL did MAX(ts). Under multi-sample keys this diverges: a key with one ancient + one fresh sample is spared by MAX(ts) but evicted by the in-memory/libSQL MIN(ts) ranking, so the three backends evict different keys. The single-sample- per-key parity matrix masks it (MIN == MAX). Fix: rank by MIN(ts) per key and update the module/fn docs to match the actual behavior. Follow-up (routed separately): the #3937 parity suite needs a multi-sample LRU case to lock this in. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * refactor(hooks-postgres): canonical typed two-table predicate schema Replace the single generic hook_predicate_counters(kind CHAR(1), id, ts, value NUMERIC) table with two explicit typed tables matching the libSQL backend's two-table model, so both durable backends share ONE logical schema (table count, column names, semantics identical; native storage types differ per backend). Canonical schema (both backends): - hooks_predicate_invocations(scope_hash, key_hash, event_id, occurred_at), PK (key_hash, event_id); in-window COUNT(*) is the invocation count. - hooks_predicate_values(scope_hash, key_hash, event_id, occurred_at, value NOT NULL), PK (key_hash, event_id); in-window SUM(value) is the sum. Postgres native types unchanged where right: BYTEA hashes, TIMESTAMPTZ occurred_at, NUMERIC value. Column renames id->event_id, ts->occurred_at to the canonical names. The kind discriminator column is gone (table identity carries it); the value: Option<Decimal> double-duty smuggling serrrfirat flagged (backend.rs:138) is eliminated — the invocation table has no value column and the value table's value is NOT NULL, with a per-table aggregate() helper so COUNT vs SUM is explicit. Migration file renamed V1__predicate_counters.sql -> V1__predicate_state.sql, still the include_str! single source of truth. Invariants preserved: MIN(occurred_at) oldest-front LRU victim rule (the 0c102a6 fix), fail-closed WindowOverflow at MAX_SAMPLES_PER_KEY, replay dedup via PK + ON CONFLICT DO NOTHING, per-tenant distinct-key quota (COUNT(DISTINCT key_hash) WHERE scope_hash, no global cap), per-key advisory lock + non-blocking victim try-lock, scope advisory lock now folds a per-table tag instead of the kind column. evict_older_than reaps both tables. SQL paths compile + skip-pass without a reachable Postgres; the typed schema needs the #3937 PG CI leg for a live run before merge. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(hooks-postgres): typed RecordPlan + fail-closed scope quota + asserting LRU test Addresses serrrfirat's post-typed-schema maintainability review on #3933. 1. Quota enforcement is now an explicit OUTCOME, not silently best-effort (backend.rs). `enforce_scope_quota` previously over-fetched a fixed 64 candidates, skipped locked victims, and committed `Ok(evicted)` even if the scope was still above MAX_KEYS_PER_TENANT — making the per-scope bound an accident of how many victims happened to be in-flight. It now requeries the distinct count + a fresh victim batch each pass and keeps evicting (deterministic oldest-front victim, per-key try-lock before delete, consistent lock order) until the cap is actually met, evicting EXACTLY the per-pass deficit so it can never over-evict. If a whole pass makes zero progress while still over cap (every stale candidate locked by an in-flight recorder) it FAILS CLOSED with Unavailable rather than committing an over-quota scope; the caller's txn rolls back and the evaluator maps it restrictively. 2. Eliminate the nullable-mode anti-pattern in the shared record path (backend.rs). The `value: Option<Decimal>` + `RecordKind` pair guarded by `debug_assert_eq` is replaced by a typed `RecordPlan` enum whose `Value` variant carries the Decimal and whose `Invocation` variant cannot. The common lock/trim/dedup/cap/quota steps run off a shared `PlanCommon`; only the INSERT column list and final aggregate dispatch on the variant. No nullable side-channel, no debug_assert invariant. 3. Adversarial LRU test now RETURNS each spawned task's backend result and asserts the allowed-outcome set explicitly, so a deadlock-detected / serialization DB error fails the test instead of passing silently as long as the task returns before the timeout. 4. New `scope_quota_is_enforced_exactly_not_best_effort` stateful test drives a sequential flood past the cap and asserts the scope holds at EXACTLY MAX_KEYS_PER_TENANT with oldest-front victims evicted — the regression guard for the best-effort BLOCKER. (This test caught an over-eviction bug in the first cut of the fix against a live Postgres.) Schema typed-columns (#2) and migration include_str! single-source (#4) were already landed in ef93722 / a784b0d on this branch; line refs in the new review pointed at the post-refactor file. Verified against a local Postgres 14: 7 lib unit + 8 adversarial + 9 contract tests green. fmt + clippy -D warnings clean crate-wide and workspace-wide. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(hooks-postgres): index-only scope-quota cover, u64 hash length prefix, defensive count clamp Address remaining review items on #3933 after the three blocker fixes landed in 741e0ec: - Expand the per-scope index on both typed tables to (scope_hash, key_hash, occurred_at) so enforce_scope_quota's COUNT(DISTINCT key_hash) and MIN(occurred_at) victim ranking run as index-only scans (gemini perf item, re-applied to the post-rewrite two-table schema). - Canonical hashing length prefix widened from 4-byte u32 (saturating) to 8-byte u64, making the usize->len conversion infallible and removing the saturation aliasing corner; keeps the serialization strictly injective. Doc updated to match. - Clamp pre_count with .max(0) before the usize cast in the fail-closed cap check, matching the codebase convention against negative-row wrap. Crate suite green (7 lib unit, 8 adversarial, 9 contract; PG-gated tests skip-pass without a live DB). fmt clean, clippy -D warnings clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(hooks-pg): address henrypark133 review on #3933 Performance (High): derive the post-insert in-window count as `pre_count + 1` instead of issuing a second COUNT, and return it directly as the invocation aggregate — removing 2 of the 3 COUNT round trips on the record() hot path. Provable under the per-key advisory lock: the dedup check proved the id absent, the cap gate passed, and the ON CONFLICT insert added exactly one in-window row, so no concurrent writer can perturb this key's count. The value aggregate stays a SUM query (not derivable from sample count). Quota gate now keys on `pre_count == 0` (equivalently the old `in_window_count == 1`). Performance (Low): pre-format every record()/reaper SQL statement once at construction (TableStatements per typed table) instead of `format!` allocating 5-6 throwaway Strings per call. Security (Low): map_pg/map_pool now log the raw DB error at warn and return a sanitized "backend unavailable" message through PredicateBackendError::Unavailable rather than leaking raw Postgres error text (which can embed connection/schema details) to callers. Tests (Medium): add evict_older_than_removes_stale_rows_from_both_tables adversarial test proving the time-based reaper deletes stale rows from both typed tables, spares in-window rows, and returns the correct count. Preserves the atomic record-and-read transaction, advisory-lock serialization, tenant-scoped keying, fail-closed quota posture, and shared identity hashing. Removed now-unused RecordPlan::table(). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(hooks-pg): sanitize quota fail-closed messages + cutoff overflow test (#3933) C-2: enforce_scope_quota built detailed Unavailable(format!(...)) payloads exposing MAX_KEYS_PER_TENANT and lock-contention state, bypassing the DB_UNAVAILABLE_MSG sanitization contract. Hoist both fail-closed messages into sanitized constants (QUOTA_CONTENDED_MSG / QUOTA_BUDGET_MSG); the operational detail now goes to tracing::debug! and the caller/evaluator only observes the error type, not the payload. T-2: add cutoff_with_overflow_window_saturates_to_now unit test asserting the Err(_) => now saturation arm of cutoff (window beyond chrono's max Duration trims nothing — conservative). Pure fn, no live Postgres. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(hooks-pg): make dual-backend compliance explicit (C-1) Address serrrfirat C-1 (Medium): the repo rule requiring both PostgreSQL and libSQL persistence backends is satisfied across the staged durable-backend series, not in this crate alone. Document the libSQL counterpart (ironclaw_hooks_libsql, PR #3936) and the cross-backend parity suite (ironclaw_hooks_parity, PR #3937) in the crate-level doc, including merge ordering, so the dependency is explicit per the reviewer's recommendation (b). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(hooks-pg): round-2 review fixes for predicate state backend (#3933) Address henrypark133 round-2 inline findings: - High: add per_value_key_sample_cap_fails_closed_under_flood mirroring the invocation flood test against record_value — fills to MAX_SAMPLES_PER_KEY, asserts WindowOverflow at cap+1, and asserts an in-window replay at the cap dedups (sum unchanged), covering the value-table INSERT and aggregate_sum cap-reject path the invocation test did not exercise. - Medium: wrap evict_older_than's two table DELETEs in one READ COMMITTED transaction so the reaper is all-or-nothing (no partial reap leaving invocation rows gone and value rows present). - Medium: add invocation_and_value_scope_lock_keys_are_distinct pinning that b"i" and b"v" lock tags derive disjoint scope advisory keys for the same tenant (pure fn, no live Postgres). - Medium: add evictions_counter_unchanged_on_window_overflow asserting evictions_observed() does not advance when a per-key cap overflow rolls back. - Low: add crates/ironclaw_hooks_postgres/AGENTS.md modeled on the sibling durable-backend crates' files. - Low: delete the advisory_lock_key(&Digest) wrapper; call advisory_lock_key_from_bytes directly at both record/eviction call sites and in the unit tests (Digest slice-coerces at zero cost). - Low: replace TableStatements::new(table, with_value_column: bool) with named constructors for_invocations / for_values per the no-boolean-mode-flags convention. Postgres-backed adversarial tests are env-gated on IRONCLAW_HOOKS_POSTGRES_URL / DATABASE_URL and skip (passing) with no DB reachable; the two new ones could not be executed against a live Postgres locally. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>

…eplaces nearai#3932) (nearai#3933) * feat(hooks): PostgresPredicateStateBackend (durable backend PR 2/4) Implements the durable PostgreSQL backend for predicate sliding-window state, satisfying the `PredicateStateBackend` contract widened to a public async trait in PR nearai#3927. New crate `ironclaw_hooks_postgres` (out-of-crate durable backend, the pattern the `contract-tests` feature on `ironclaw_hooks` was designed for; mirrors `ironclaw_reborn_event_store`). Keeps the Postgres dependency surface out of the hook framework itself. Correctness: - Atomic record-and-read: each `record_*` runs in one READ COMMITTED tx guarded by a transaction-scoped advisory lock on the bucket. The advisory lock (not REPEATABLE READ) serializes same-key writers by BLOCKING the second writer rather than aborting it with a serialization failure — no caller retry loop. Trim, insert, cap-evict, aggregate all share the one tx. - Cross-host replay dedup via PRIMARY KEY (key_hash, id) + INSERT ON CONFLICT DO NOTHING — exact regardless of clock skew. - Running-sum consistency under eviction: cap eviction re-aggregates inside the same tx so the returned sum reflects dropped rows. - Per-key sample cap (MAX_SAMPLES_PER_KEY, drop-oldest) and per-scope distinct-key LRU quota (MAX_KEYS_PER_TENANT) match the in-memory backend. Scope-quota enforcement takes a second advisory lock in the disjoint (int8) lock space so concurrent new-key inserts in a scope don't under-evict. Schema: single `hook_predicate_counters` table, BYTEA blake3 hash keys, TEXT id column (NOT uuid — Postgres uuid rejects the 64-char blake3 hex digest; resolves the nearai#3635 docs/schema contradiction in favor of TEXT). Idempotent CREATE ... IF NOT EXISTS applied via run_migrations(); the per-crate pattern, not the legacy main-binary refinery migrations. DB-clock decision: window comparison basis is the caller's `now` (the same Utc::now() the in-memory backend trusts), making this a drop-in under the deterministic-clock contract harness. Replay dedup and atomicity do NOT depend on clock agreement. Tests (env-gated on IRONCLAW_HOOKS_POSTGRES_URL / DATABASE_URL, skip when absent; schema-isolated so the two test binaries can share one DB): - All 8 shared contract functions via the `contract-tests` harness. - Adversarial: two-host write storm (no count desync), cross-host replay (count + value), per-key cap flood, per-scope LRU under concurrent insert pressure across two hosts. - 4 hashing unit tests (injective length-prefixed serialization). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(hooks-postgres): take victim-key advisory lock during scope-LRU eviction (deadlock + race) enforce_scope_quota deleted victim buckets' rows while holding only the per-scope advisory lock, never the victims' per-key advisory lock. That broke the per-bucket serialization guarantee in two ways: * Torn aggregate: the LRU pass could delete key B's rows while another transaction was recording B under B's own per-key lock, so the recorder's COUNT/SUM straddled a delete it never serialized against. * Deadlock: a recorder of victim key B holds B's per-key lock and then waits on the scope lock inside its own quota pass, while the LRU transaction holds the scope lock and waits on B's row locks to delete them -> cycle. Fix: evict victims one at a time, each guarded by that victim key's per-key advisory lock acquired with the NON-blocking pg_try_advisory_xact_lock. An in-flight victim (lock already held by a concurrent recorder) is skipped, not waited on, so the deadlock cycle cannot form and eviction never deletes rows out from under an unserialized transaction. Candidates are over-fetched beyond the evict count so skips still meet the per-scope quota. Fail-closed WindowOverflow semantics and the dedup/atomicity invariants are unchanged. Adds a lib unit test pinning the eviction try-lock key derivation equal to the recorder's lock key (lock-acquisition invariant, no DB needed), a deterministic raw-SQL regression test proving the try-lock on an in-flight victim returns immediately rather than blocking, and a concurrency stress test asserting no deadlock and no torn aggregate. The integration tests gate on IRONCLAW_HOOKS_POSTGRES_URL/DATABASE_URL and were verified against a real Postgres 14. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * refactor(hooks-postgres): make migration SQL the single source via include_str! firat (PR nearai#3933 maintainability audit): schema.rs embedded a hand-copied DDL const documented as "byte-compatible" with migrations/V1__predicate_counters.sql, with nothing enforcing equivalence — silent drift risk. Pull the DDL directly from the .sql file via include_str! so there is exactly one copy. batch_execute tolerates the file's leading -- comment block, so no statement splitting is needed. run_migrations is exercised by the contract/adversarial suites, confirming the included SQL applies. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(hooks-postgres): rank LRU eviction victims by MIN(ts), matching in-memory/libSQL serrrfirat (PR nearai#3933): the Postgres scope-LRU victim query ranked keys by MAX(ts) (most-recent activity) while the in-memory backend evicts by each bucket's OLDEST retained sample (entries.front() + min_by_key) and libSQL does the same (oldest-front). The doc comment claimed oldest-front but the SQL did MAX(ts). Under multi-sample keys this diverges: a key with one ancient + one fresh sample is spared by MAX(ts) but evicted by the in-memory/libSQL MIN(ts) ranking, so the three backends evict different keys. The single-sample- per-key parity matrix masks it (MIN == MAX). Fix: rank by MIN(ts) per key and update the module/fn docs to match the actual behavior. Follow-up (routed separately): the nearai#3937 parity suite needs a multi-sample LRU case to lock this in. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * refactor(hooks-postgres): canonical typed two-table predicate schema Replace the single generic hook_predicate_counters(kind CHAR(1), id, ts, value NUMERIC) table with two explicit typed tables matching the libSQL backend's two-table model, so both durable backends share ONE logical schema (table count, column names, semantics identical; native storage types differ per backend). Canonical schema (both backends): - hooks_predicate_invocations(scope_hash, key_hash, event_id, occurred_at), PK (key_hash, event_id); in-window COUNT(*) is the invocation count. - hooks_predicate_values(scope_hash, key_hash, event_id, occurred_at, value NOT NULL), PK (key_hash, event_id); in-window SUM(value) is the sum. Postgres native types unchanged where right: BYTEA hashes, TIMESTAMPTZ occurred_at, NUMERIC value. Column renames id->event_id, ts->occurred_at to the canonical names. The kind discriminator column is gone (table identity carries it); the value: Option<Decimal> double-duty smuggling serrrfirat flagged (backend.rs:138) is eliminated — the invocation table has no value column and the value table's value is NOT NULL, with a per-table aggregate() helper so COUNT vs SUM is explicit. Migration file renamed V1__predicate_counters.sql -> V1__predicate_state.sql, still the include_str! single source of truth. Invariants preserved: MIN(occurred_at) oldest-front LRU victim rule (the 0c102a6 fix), fail-closed WindowOverflow at MAX_SAMPLES_PER_KEY, replay dedup via PK + ON CONFLICT DO NOTHING, per-tenant distinct-key quota (COUNT(DISTINCT key_hash) WHERE scope_hash, no global cap), per-key advisory lock + non-blocking victim try-lock, scope advisory lock now folds a per-table tag instead of the kind column. evict_older_than reaps both tables. SQL paths compile + skip-pass without a reachable Postgres; the typed schema needs the nearai#3937 PG CI leg for a live run before merge. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(hooks-postgres): typed RecordPlan + fail-closed scope quota + asserting LRU test Addresses serrrfirat's post-typed-schema maintainability review on nearai#3933. 1. Quota enforcement is now an explicit OUTCOME, not silently best-effort (backend.rs). `enforce_scope_quota` previously over-fetched a fixed 64 candidates, skipped locked victims, and committed `Ok(evicted)` even if the scope was still above MAX_KEYS_PER_TENANT — making the per-scope bound an accident of how many victims happened to be in-flight. It now requeries the distinct count + a fresh victim batch each pass and keeps evicting (deterministic oldest-front victim, per-key try-lock before delete, consistent lock order) until the cap is actually met, evicting EXACTLY the per-pass deficit so it can never over-evict. If a whole pass makes zero progress while still over cap (every stale candidate locked by an in-flight recorder) it FAILS CLOSED with Unavailable rather than committing an over-quota scope; the caller's txn rolls back and the evaluator maps it restrictively. 2. Eliminate the nullable-mode anti-pattern in the shared record path (backend.rs). The `value: Option<Decimal>` + `RecordKind` pair guarded by `debug_assert_eq` is replaced by a typed `RecordPlan` enum whose `Value` variant carries the Decimal and whose `Invocation` variant cannot. The common lock/trim/dedup/cap/quota steps run off a shared `PlanCommon`; only the INSERT column list and final aggregate dispatch on the variant. No nullable side-channel, no debug_assert invariant. 3. Adversarial LRU test now RETURNS each spawned task's backend result and asserts the allowed-outcome set explicitly, so a deadlock-detected / serialization DB error fails the test instead of passing silently as long as the task returns before the timeout. 4. New `scope_quota_is_enforced_exactly_not_best_effort` stateful test drives a sequential flood past the cap and asserts the scope holds at EXACTLY MAX_KEYS_PER_TENANT with oldest-front victims evicted — the regression guard for the best-effort BLOCKER. (This test caught an over-eviction bug in the first cut of the fix against a live Postgres.) Schema typed-columns (#2) and migration include_str! single-source (#4) were already landed in ef93722 / a784b0d on this branch; line refs in the new review pointed at the post-refactor file. Verified against a local Postgres 14: 7 lib unit + 8 adversarial + 9 contract tests green. fmt + clippy -D warnings clean crate-wide and workspace-wide. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(hooks-postgres): index-only scope-quota cover, u64 hash length prefix, defensive count clamp Address remaining review items on nearai#3933 after the three blocker fixes landed in 741e0ec: - Expand the per-scope index on both typed tables to (scope_hash, key_hash, occurred_at) so enforce_scope_quota's COUNT(DISTINCT key_hash) and MIN(occurred_at) victim ranking run as index-only scans (gemini perf item, re-applied to the post-rewrite two-table schema). - Canonical hashing length prefix widened from 4-byte u32 (saturating) to 8-byte u64, making the usize->len conversion infallible and removing the saturation aliasing corner; keeps the serialization strictly injective. Doc updated to match. - Clamp pre_count with .max(0) before the usize cast in the fail-closed cap check, matching the codebase convention against negative-row wrap. Crate suite green (7 lib unit, 8 adversarial, 9 contract; PG-gated tests skip-pass without a live DB). fmt clean, clippy -D warnings clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(hooks-pg): address henrypark133 review on nearai#3933 Performance (High): derive the post-insert in-window count as `pre_count + 1` instead of issuing a second COUNT, and return it directly as the invocation aggregate — removing 2 of the 3 COUNT round trips on the record() hot path. Provable under the per-key advisory lock: the dedup check proved the id absent, the cap gate passed, and the ON CONFLICT insert added exactly one in-window row, so no concurrent writer can perturb this key's count. The value aggregate stays a SUM query (not derivable from sample count). Quota gate now keys on `pre_count == 0` (equivalently the old `in_window_count == 1`). Performance (Low): pre-format every record()/reaper SQL statement once at construction (TableStatements per typed table) instead of `format!` allocating 5-6 throwaway Strings per call. Security (Low): map_pg/map_pool now log the raw DB error at warn and return a sanitized "backend unavailable" message through PredicateBackendError::Unavailable rather than leaking raw Postgres error text (which can embed connection/schema details) to callers. Tests (Medium): add evict_older_than_removes_stale_rows_from_both_tables adversarial test proving the time-based reaper deletes stale rows from both typed tables, spares in-window rows, and returns the correct count. Preserves the atomic record-and-read transaction, advisory-lock serialization, tenant-scoped keying, fail-closed quota posture, and shared identity hashing. Removed now-unused RecordPlan::table(). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(hooks-pg): sanitize quota fail-closed messages + cutoff overflow test (nearai#3933) C-2: enforce_scope_quota built detailed Unavailable(format!(...)) payloads exposing MAX_KEYS_PER_TENANT and lock-contention state, bypassing the DB_UNAVAILABLE_MSG sanitization contract. Hoist both fail-closed messages into sanitized constants (QUOTA_CONTENDED_MSG / QUOTA_BUDGET_MSG); the operational detail now goes to tracing::debug! and the caller/evaluator only observes the error type, not the payload. T-2: add cutoff_with_overflow_window_saturates_to_now unit test asserting the Err(_) => now saturation arm of cutoff (window beyond chrono's max Duration trims nothing — conservative). Pure fn, no live Postgres. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(hooks-pg): make dual-backend compliance explicit (C-1) Address serrrfirat C-1 (Medium): the repo rule requiring both PostgreSQL and libSQL persistence backends is satisfied across the staged durable-backend series, not in this crate alone. Document the libSQL counterpart (ironclaw_hooks_libsql, PR nearai#3936) and the cross-backend parity suite (ironclaw_hooks_parity, PR nearai#3937) in the crate-level doc, including merge ordering, so the dependency is explicit per the reviewer's recommendation (b). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(hooks-pg): round-2 review fixes for predicate state backend (nearai#3933) Address henrypark133 round-2 inline findings: - High: add per_value_key_sample_cap_fails_closed_under_flood mirroring the invocation flood test against record_value — fills to MAX_SAMPLES_PER_KEY, asserts WindowOverflow at cap+1, and asserts an in-window replay at the cap dedups (sum unchanged), covering the value-table INSERT and aggregate_sum cap-reject path the invocation test did not exercise. - Medium: wrap evict_older_than's two table DELETEs in one READ COMMITTED transaction so the reaper is all-or-nothing (no partial reap leaving invocation rows gone and value rows present). - Medium: add invocation_and_value_scope_lock_keys_are_distinct pinning that b"i" and b"v" lock tags derive disjoint scope advisory keys for the same tenant (pure fn, no live Postgres). - Medium: add evictions_counter_unchanged_on_window_overflow asserting evictions_observed() does not advance when a per-key cap overflow rolls back. - Low: add crates/ironclaw_hooks_postgres/AGENTS.md modeled on the sibling durable-backend crates' files. - Low: delete the advisory_lock_key(&Digest) wrapper; call advisory_lock_key_from_bytes directly at both record/eviction call sites and in the unit tests (Digest slice-coerces at zero cost). - Low: replace TableStatements::new(table, with_value_column: bool) with named constructors for_invocations / for_values per the no-boolean-mode-flags convention. Postgres-backed adversarial tests are env-gated on IRONCLAW_HOOKS_POSTGRES_URL / DATABASE_URL and skip (passing) with no DB reachable; the two new ones could not be executed against a live Postgres locally. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>

zmanian and others added 2 commits May 23, 2026 05:06

github-actions Bot added size: XL 500+ changed lines risk: medium Business logic, config, or moderate-risk modules contributor: core 20+ merged PRs labels May 23, 2026

gemini-code-assist Bot reviewed May 23, 2026

View reviewed changes

chatgpt-codex-connector Bot reviewed May 23, 2026

View reviewed changes

zmanian force-pushed the hooks-predicate-backend-trait-widening branch from 3718140 to baa6688 Compare May 23, 2026 12:49

zmanian deleted the branch hooks-predicate-backend-trait-widening May 23, 2026 12:50

zmanian closed this May 23, 2026

zmanian mentioned this pull request May 23, 2026

feat(hooks): PostgresPredicateStateBackend (durable backend PR 2/4, replaces #3932) #3933

Merged

Navigation Menu

feat(hooks): PostgresPredicateStateBackend (durable backend PR 2/4) - #3932

feat(hooks): PostgresPredicateStateBackend (durable backend PR 2/4)#3932
zmanian wants to merge 2 commits into
hooks-predicate-backend-trait-wideningfrom
hooks-predicate-backend-postgres

zmanian commented May 23, 2026

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

gemini-code-assist Bot May 23, 2026

Uh oh!

gemini-code-assist Bot May 23, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

chatgpt-codex-connector Bot May 23, 2026

Uh oh!

zmanian commented May 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

		//! running prune + insert + aggregate inside one `REPEATABLE READ`
		//! transaction, also clock-independent.

Conversation

zmanian commented May 23, 2026

What

Crate placement

Atomic-tx shape

DB-clock decision

id-column type (Codex #3635 finding)

Test plan

Divergence note

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot May 23, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot May 23, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot May 23, 2026

Choose a reason for hiding this comment

Uh oh!

zmanian commented May 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant