From 602997bcdbe40c910d987c474cefd898d7af9fce Mon Sep 17 00:00:00 2001 From: Hein Date: Mon, 14 Sep 2026 23:10:29 +0200 Subject: [PATCH] docs(plan): add broker audit and recommendations --- plan/docs/recommendations.md | 332 +++++++++++++++++++++++++++++++++++ plan/docs/security_audit.md | 150 ++++++++++++++++ 2 files changed, 482 insertions(+) create mode 100644 plan/docs/recommendations.md create mode 100644 plan/docs/security_audit.md diff --git a/plan/docs/recommendations.md b/plan/docs/recommendations.md new file mode 100644 index 0000000..33b88a3 --- /dev/null +++ b/plan/docs/recommendations.md @@ -0,0 +1,332 @@ +# PostgreSQL Schema and Function Recommendations + +**Review date:** 2026-09-14 +**Scope:** `pkg/broker/install/sql` tables and functions, including their interaction with the Go worker. + +## Conclusion + +The current schema is a credible internal prototype, but it is not yet a reliable production job queue. It has correctness problems that can strand jobs, lacks a safe RLS/execution-identity design, and needs standard operational queue features such as leases, retries, and migrations. + +It is suitable only for a controlled environment with a single trusted operator and simple successful jobs until the priority items below are addressed. + +## Prioritized recommendations + +| Priority | Problem | Recommendation | +| --- | --- | --- | +| Critical | A failed job is marked failed inside `broker_run`, then the function returns a nonzero result. The Go worker rolls back its transaction on nonzero results, so the failed update is rolled back and the job remains `running` forever. | Treat an expected job failure as a committed job outcome, while reserving nonzero function errors for infrastructure failures; alternatively persist the failure in a separate transaction. Add stale-running-job recovery. | +| Critical | The notification handler passes a newly pending job directly to `broker_run`, but `broker_run` requires status `running`. The notification attempt fails and polling eventually processes the job. | Use NOTIFY only to wake workers. Every work attempt must claim a job through `broker_get` and `FOR UPDATE SKIP LOCKED`. | +| Critical | Arbitrary queued SQL executes with the broker role. This cannot be made safe for multi-tenant/RLS use without an explicit execution model. | Prefer approved job procedures or job types with structured arguments. Add immutable tenant and execution-principal references. Use a least-privileged runtime role that does not own tenant tables and lacks `BYPASSRLS`. | +| High | Active-instance registration is race-prone and the Go process can reuse another process's instance ID. | Fail a second startup. Use a session-held PostgreSQL advisory lock for exclusive ownership, with the instance table retained for observability and heartbeats. | +| High | Jobs have no retry policy, lease expiry, dead-letter state, idempotency key, attempt counter, or backoff. | Add `attempt_count`, `max_attempts`, `available_at`, `leased_at`, `lease_expires_at`, `lease_token`, and terminal/dead-letter handling. | +| High | Dependencies use mutable, non-unique names and treat running, failed, or cancelled dependencies as satisfied. | Replace `depends_on text[]` with `broker_job_dependency(job_id, depends_on_job_id)`. Permit execution only when all dependencies completed successfully, and define behaviour for failed dependencies. | +| High | Schema installation is not idempotent because existing triggers cause reinstallation failures. There is no migration/version tracking. | Use ordered, transactional migrations with a schema-version table. Make trigger creation idempotent or recreate triggers safely. | +| Medium | Job execution is inside the claim transaction. It is atomic but holds locks through arbitrary job SQL and prevents transaction-controlling work. | Explicitly choose a short lease/ack design with idempotent jobs, or document the atomic transaction limitation and restrict job types accordingly. | +| Medium | `run_as` is unused. `broker_set` permits persistent session changes and its `search_path` branch is unsafe. | Remove identity controls until fully designed. Use only whitelisted transaction-local settings via `SET LOCAL` or `set_config(..., true)`. | +| Medium | Existing indexes do not exactly serve queue claiming by queue, pending status, descending priority, and creation time. | Add and validate a partial claim index: `(job_queue, job_priority DESC, created_at, id_broker_jobs) WHERE complete_status = 0`. | +| Medium | Objects are unqualified and depend on `search_path`, complicating future privilege/RLS work. | Create a dedicated `broker` schema, schema-qualify all references, revoke `PUBLIC` access, and grant only the roles that need each operation. | + +## Correctness failure to fix first + +The present worker and function behavior can permanently strand a failed job: + +```text +broker_get → marks job running +broker_run → job SQL fails → marks job failed, returns error +Go worker → sees error → ROLLBACK +Result → failed update is rolled back; job stays running indefinitely +``` + +Related implementation: + +- `pkg/broker/install/sql/procedures/02_broker_run.sql` +- `pkg/broker/worker/worker.go` + +## Recommended target model + +```text +broker_job + id, tenant_id, job_type, arguments jsonb, status, + priority, available_at, attempt_count, max_attempts, + lease_token, leased_at, lease_expires_at, + created_by, completed_at, result, error + +broker_job_dependency + job_id, depends_on_job_id +``` + +Use a dedicated `broker` schema and separate roles: + +1. A non-login migration/schema-owner role. +2. A least-privileged broker runtime role, with no `BYPASSRLS` and no ownership of tenant tables. +3. Narrowly scoped enqueue/application roles. + +RLS must use immutable `tenant_id` and a transaction-local, trusted tenant context. It must not be driven by arbitrary queued SQL or an unvalidated `run_as` field. + +## Delivery order + +1. Correct rollback/error outcomes and change notifications into worker wake-ups. +2. Establish the role, privilege, RLS, and tenant-context design. +3. Implement leases, retries, stale-job recovery, and idempotency. +4. Replace installation scripts with idempotent versioned migrations. +5. Add integration coverage for failed jobs, duplicate starts, stale leases, RLS isolation, dependency outcomes, and repeat migration runs. + +## Cleanup and maintenance + +The current design only appends history. Completed jobs, error messages, execution results, and historical broker instances will grow indefinitely. This will eventually increase index size, autovacuum work, backup duration, and query latency. Maintenance must be a designed, observable capability rather than an ad hoc deletion script. + +### Retention policy + +Define retention separately for successful jobs, failed/dead-letter jobs, job output/error text, scheduled jobs, and historical instance records. Retention must account for business audit, incident investigation, tenant requirements, and legal obligations before data is deleted. + +Recommended implementation: + +1. Store a terminal timestamp and an immutable tenant identifier on every job. +2. Add an archival state or archive table when completed job history remains useful beyond the online retention window. +3. Implement a broker-owned maintenance procedure that deletes or archives only terminal records older than a configured retention period, in bounded batches. +4. Enforce tenant-aware RLS and authorization for retained job payloads, results, and error messages; these fields can contain sensitive data. +5. Emit counts and oldest-record timestamps before and after each maintenance run. + +### Runtime reconciliation + +Add scheduled maintenance operations for: + +1. Expiring leases held by crashed workers and returning eligible jobs to the queue. +2. Marking stale broker instances inactive after a defined heartbeat timeout, without allowing accidental takeover of a live owner. +3. Retrying transient failures according to backoff and maximum-attempt rules; routing exhausted jobs to a dead-letter state. +4. Identifying jobs that have been `running` longer than their configured maximum duration. +5. Detecting queue backlog, oldest pending job age, retry/dead-letter count, and worker heartbeat health. + +### Database health + +Frequent status updates produce dead tuples and index churn. Operational documentation should specify: + +1. Autovacuum monitoring and table-specific tuning after measuring production load. +2. `ANALYZE` and index-health monitoring for the job claim query. +3. Bounded deletion batches to avoid long-running transactions, replication lag, and lock pressure. +4. Optional time partitioning for large, retention-oriented job history tables; do not partition prematurely for a small deployment. +5. Backup/restore testing and a migration rollback/recovery procedure. + +### Guardrails + +Never purge pending, leased, running, failed, or dead-letter jobs merely because they are old. Automated cleanup must operate only on explicitly eligible terminal states, use a documented retention configuration, and be dry-run capable before destructive execution. + +## Scale readiness: millions of reads and writes + +The current tables can support modest workloads, but they are not designed for sustained millions of job events. In particular, updating a single wide `broker_jobs` row through every lifecycle state creates table and index churn; retaining results and errors in that same hot table makes every queue scan increasingly expensive; and the current one-active-broker rule prevents horizontal worker scaling within a database. + +### Target storage model + +Separate the hot queue from append-heavy operational history: + +```text +broker_job -- current mutable queue state; narrow and hot + id, tenant_id, queue_id, status, priority, available_at, + attempt_count, lease_token, lease_expires_at, created_at + +broker_job_payload -- command/type and structured arguments; accessed after claim + job_id, job_type, arguments jsonb, created_by + +broker_job_execution -- append-only attempts, results, errors, timings + id, job_id, attempt_no, state, started_at, completed_at, error, result + +broker_job_event -- optional append-only audit/event stream + id, job_id, tenant_id, event_type, occurred_at, metadata jsonb +``` + +Keep the claim path narrow: it should not scan or update large payload/result columns. Treat result/error retention as a separate policy and keep sensitive fields protected by tenant-aware RLS. + +### Claim and indexing strategy + +1. Claim jobs atomically with `FOR UPDATE SKIP LOCKED` and an immediate lease update, ideally in one statement or a tightly scoped function. Do not select a job and later claim it in a separate transaction. +2. Use a partial index for claimable work, such as `(queue_id, priority DESC, available_at, id) WHERE status = 'pending'`. Validate the exact form with `EXPLAIN (ANALYZE, BUFFERS)` against representative data. +3. Keep status and queue keys compact (`smallint`/foreign key where suitable) and avoid indexing every mutable column. Every index on frequently updated rows adds write amplification. +4. Support bounded batch claims only after measuring contention and fairness; never use offset pagination for queue work. +5. Add idempotency keys with an appropriate unique constraint so client retries do not create duplicate jobs. + +### Partitioning and retention + +Partition large append-only execution/event history by time, normally monthly at first, so retention can detach/archive/drop old partitions efficiently. Consider partitioning the mutable job table only after measurement: its unique-key and foreign-key constraints become more complex, and its partition key must be reflected in uniqueness design. + +For very large multi-tenant workloads, assess hash partitioning by tenant or queue only when it demonstrably reduces contention; it is not a substitute for correct indexes and bounded retention. + +### Horizontal operation + +Millions of events require multiple broker processes and workers to claim work concurrently. Replace the current global single-instance restriction with one of these deliberate models: + +1. Multiple stateless workers sharing the same queue and using row leases plus `SKIP LOCKED`; or +2. One elected scheduler/maintenance leader with many independent queue workers. + +The leader lock must protect only leader duties such as schedule creation and reconciliation. It must not serialize normal job claiming. Worker ownership must be represented by lease tokens and expiry, not a permanent single active instance row. + +### PostgreSQL operating requirements + +1. Use a dedicated, session-persistent connection for `LISTEN`; transaction-pooling proxies cannot safely carry listener state. Use separate pooled connections for claims and execution. +2. Treat `NOTIFY` as a low-latency wake-up only. Its payload is size-limited and notification delivery must not be the sole source of truth; periodic/bounded polling remains necessary. +3. Tune connection-pool sizes per database capacity, rather than multiplying workers and connections without a budget. +4. Monitor queue depth, oldest-ready-job age, claim latency, execution latency, lock waits, dead tuples, autovacuum progress, index size, WAL volume, replication lag, and database connection saturation. +5. Load-test realistic payload sizes, tenant distributions, job durations, retry rates, and failure modes before selecting partitions, worker counts, or autovacuum settings. + +### Capacity acceptance criteria + +Before production rollout, establish and automate tests for the intended workload: + +1. Sustained and burst enqueue/claim/complete rates at the target million-event volume. +2. Correctness under concurrent workers, duplicate client submissions, broker crashes, worker lease expiry, and notification loss. +3. RLS isolation across concurrent tenants at load. +4. Stable p95/p99 claim latency and bounded table/index growth over a retention cycle. +5. Successful archival/purge, backup, restore, and migration exercises using production-scale representative data. + +## Additional platform improvements + +### Delivery semantics and transactional outbox + +Define at-least-once delivery as the broker's explicit contract. Exactly-once execution is generally not achievable across process and database failures, so job handlers must be idempotent. Add idempotency keys and enforce them with an appropriate unique constraint. + +For work created as part of an application database change, implement a transactional outbox. The application transaction should record both the business change and the intent to enqueue work; a reliable dispatcher then creates or exposes the broker job after commit. This prevents a committed business change from losing its corresponding job due to an application crash between separate writes. + +### Fairness, quotas, cancellation, and time limits + +Add per-tenant and per-queue concurrency caps, rate limits, and quotas so one tenant cannot exhaust workers or storage. Implement priority ageing so a continuous stream of high-priority jobs does not permanently starve lower-priority work. + +Add a cancellation state and cooperative cancellation protocol. Jobs should support deadlines, lease-aware cancellation checks, and configured `statement_timeout` and `lock_timeout` values. The broker must distinguish a user cancellation from a timeout, execution failure, or expired lease. + +### Cross-queue dependency scheduling + +The broker must support dependencies between jobs in different queues without allowing a blocked job to stall a queue. Queue membership determines which worker can claim a job; dependency eligibility determines whether that job is currently runnable. They are independent concerns. + +Replace the current `depends_on text[]` column with a normalized dependency table: + +```text +broker_job_dependency + job_id bigint not null references broker_job(id) + depends_on_job_id bigint not null references broker_job(id) + created_at timestamptz not null + primary key (job_id, depends_on_job_id) + check (job_id <> depends_on_job_id) +``` + +When claiming work for one queue, the claim query must select only jobs for which every dependency has completed successfully. Its eligibility predicate should have this shape: + +```sql +NOT EXISTS ( + SELECT 1 + FROM broker_job_dependency d + JOIN broker_job dependency ON dependency.id = d.depends_on_job_id + WHERE d.job_id = candidate.id + AND dependency.status <> 'completed' +) +``` + +Combined with `ORDER BY priority DESC, available_at, id FOR UPDATE SKIP LOCKED`, this lets a worker skip blocked work and claim the next runnable job in its own queue. A dependency can therefore be in any other queue without blocking unrelated jobs. + +Required behavior and safeguards: + +1. A job may run only after every dependency is `completed`; `running`, `pending`, `failed`, `cancelled`, and `dead_letter` are not success. +2. When a dependency reaches a non-success terminal state, mark dependent jobs `blocked` or `cancelled` with a recorded reason. Do not leave them pending forever. +3. Reject self-dependencies and detect cycles when dependencies are created. Use a recursive validation query or a transactionally maintained acyclic graph policy. +4. Create jobs and their dependency rows atomically in one transaction; a job must never become visible without its full dependency set. +5. Index both lookup directions: primary key `(job_id, depends_on_job_id)` for eligibility checks and `(depends_on_job_id, job_id)` for propagating a dependency completion/failure to dependents. +6. Restrict dependencies to the same tenant by default. Cross-tenant dependencies require an explicit sharing/authorization model compatible with RLS. +7. On dependency completion or terminal failure, notify affected queues as an optimization. Polling/claim scans remain the source of truth, so missed notifications do not affect correctness. + +Test this with dependencies across multiple queues, multiple workers, priority ordering, concurrent completion, failed/cancelled prerequisites, cycles, duplicate dependency creation, RLS isolation, and notification loss. + +### Run groups and named dependency targets + +Jobs must support a `run_group` name, and a dependency declaration must be able to target either a job name or a run-group name. Names are useful at the API boundary, but the broker must resolve them to immutable IDs for reliable runtime scheduling. + +Recommended model: + +```text +broker_run_group + id, tenant_id, name, state, completion_policy, sealed_at, created_at + unique (tenant_id, name) + +broker_job + id, tenant_id, run_group_id, job_name, queue_id, status, ... + unique (run_group_id, job_name) + +broker_job_dependency + job_id, dependency_kind, depends_on_job_id, depends_on_group_id + -- dependency_kind is 'job' or 'group'; exactly one target ID is non-null +``` + +The enqueue API may accept `depends_on: ['daily-import', 'validate-customers']`, but each reference must be explicitly typed as a job or group when names could overlap. For example: + +```json +[ + { "kind": "group", "name": "daily-import" }, + { "kind": "job", "group": "daily-import", "name": "validate-customers" } +] +``` + +Required semantics: + +1. A job dependency is satisfied only when its referenced job is `completed`. +2. A group dependency is satisfied only when the group is sealed and all jobs in that group are `completed`, unless an explicitly selected alternative completion policy says otherwise. +3. An empty sealed group completes successfully by default; this should be documented and configurable only if a different business rule is required. +4. If any member of a prerequisite group fails, is cancelled, or reaches dead-letter status, propagate a `blocked` or `cancelled` terminal outcome to dependent jobs, with the failed member recorded as the reason. +5. A group must be sealed before jobs outside it may rely on its completion. This prevents adding a new job after a dependent has already started and silently changing the meaning of the dependency. +6. Group and job names are tenant-scoped. Job names are unique only within their run group, so an unqualified job-name dependency is invalid unless the API supplies its target group or a separately tenant-unique job key. +7. Create a run group, its jobs, and all resolvable dependency edges in one transaction. Resolve names to IDs during that transaction and reject missing references, ambiguous names, self-dependencies, and cycles. +8. If future references are genuinely required, model them as a separate, explicit deferred-reference state with a deadline and reconciliation process. Do not silently treat a missing job or group name as an already satisfied dependency. + +Queue claiming remains unchanged: a worker considers only jobs in its own queue, then evaluates the resolved job/group dependencies. An unresolved or incomplete group/job is ineligible, so the worker skips it and moves to the next runnable candidate in that queue. + +Index `broker_run_group` on `(tenant_id, name)`, jobs on `(run_group_id, job_name)`, dependency targets in both directions, and maintain group completion state incrementally or through an indexed aggregate query. Avoid scanning every historical group member during every queue claim at high volume. + +### Typed payloads and contracts + +Replace raw SQL text with versioned job types and validated structured arguments, normally `jsonb`. Establish payload-size limits and use external object/blob storage when job input or output is too large for a hot operational table. + +Define stable enqueue, status, retry, and cancellation API contracts. Document terminal states, error codes, compatibility expectations, and job-type versioning so clients can evolve safely. + +### Scheduling + +Jobs must support cron-style schedules for hourly, daily, weekly, and more complex recurring execution. Implement scheduling fully rather than retaining the current unused schedule schema. + +Use an explicit schedule model: + +```text +broker_schedule + id, tenant_id, name, cron_expression, timezone, enabled, + job_template/type and arguments, queue_id, priority, + misfire_policy, next_run_at, last_run_at, created_at, updated_at + unique (tenant_id, name) + +broker_schedule_run + id, schedule_id, scheduled_for, run_group_id, job_id, + state, created_at + unique (schedule_id, scheduled_for) +``` + +Required behavior: + +1. Define and document a single cron dialect, including whether it has five fields (minute through day-of-week) or seconds. Validate expressions at create/update time using the same parser the scheduler uses. +2. Require an IANA timezone, such as `Africa/Johannesburg`, on every schedule. Do not use a server-local default. Compute `next_run_at` in the configured timezone and persist it as `timestamptz`. +3. Materialize every due occurrence using a unique `(schedule_id, scheduled_for)` key. This makes scheduler retries and leader failover idempotent and prevents duplicate hourly/daily/weekly jobs. +4. Define a per-schedule misfire policy for downtime: `skip`, `run_once`, or bounded `catch_up`. Establish a maximum catch-up count/window so an extended outage cannot flood the queue. +5. Define daylight-saving behavior explicitly: for a local time that occurs twice, run once or twice by policy; for a nonexistent local time, skip or run at the next valid instant by policy. Test both cases. +6. Have only an elected scheduler leader create due runs. Use a database advisory lock or equivalent short-lived leader lease; normal workers must remain horizontally scalable and independent. +7. Each occurrence should create a new immutable job or run group, for example `schedule::`. A run must never mutate a previous occurrence's job. +8. For recurring workflows with multiple dependent jobs, create an occurrence-specific run group and resolve dependency edges only within that occurrence. A daily run must not accidentally depend on a job from yesterday's or tomorrow's run. +9. Respect tenant RLS when managing schedules and generated jobs. The scheduler needs a controlled tenant execution context, not unrestricted access to all tenant data. +10. Publish schedule health metrics: next-run lateness, due-run creation latency, misfires, disabled schedules, duplicate-prevention conflicts, and generated-job failures. + +The scheduler should use database time when deciding what is due, or otherwise use one explicitly controlled time source. Do not rely on each broker host's local clock. + +### Observability and operations + +Include job ID, tenant ID, request/correlation ID, job type, attempt number, and execution owner in structured logs. Publish metrics and traces for enqueue, claim, execution, retry, failure, cancellation, lease expiry, queue depth, and oldest-ready-job age. + +Define dashboards and alerts for backlog growth, SLA breach risk, dead-letter growth, worker health, database saturation, lock waits, autovacuum lag, replication lag, and maintenance failure. + +### Resilience, testing, and deployment + +Test backup and restore, active-worker crashes, duplicate notifications, broker upgrades, PostgreSQL failover, notification loss, and migration recovery. Add property and concurrency tests for claim correctness, permission/RLS tests, schema migration tests, and production-shaped load/soak tests. + +Provide health and readiness probes, graceful draining on shutdown, secret-management integration, strict configuration validation, and a safe zero-downtime upgrade process. + +### Data governance + +Define job payload, result, and error-message retention; PII redaction; audit access; tenant data export; and tenant deletion rules. Apply these controls consistently to hot tables, archives, logs, traces, backups, and dead-letter records. diff --git a/plan/docs/security_audit.md b/plan/docs/security_audit.md new file mode 100644 index 0000000..669ac4d --- /dev/null +++ b/plan/docs/security_audit.md @@ -0,0 +1,150 @@ +# PostgreSQL Broker Security and Readiness Audit + +**Audit date:** 2026-09-14 +**Scope:** Repository review of the PostgreSQL broker source, schema, build/release automation, documentation, and available test suite. + +## Executive summary + +The project is an early, functional prototype and is not production-ready. The working tree was clean at review time. Development activity is limited to five commits from 2–3 January 2026, with no release tag. + +Implemented capabilities include a Go CLI, multi-database configuration, embedded schema installation, PostgreSQL LISTEN/NOTIFY, queue/worker processing, instance heartbeats, CI workflows, and a single end-to-end happy-path test. + +The highest-priority work is establishing a safe execution security model, fixing single-instance ownership, making installation idempotent, and adding recovery-focused test coverage. + +## Findings + +### Critical: arbitrary SQL privilege escalation + +Any role allowed to call `broker_add_job` can enqueue SQL that the broker later executes under the broker database role. PostgreSQL functions are executable by `PUBLIC` by default unless privileges are explicitly revoked. This can allow a lower-privileged caller to cause arbitrary SQL to run with the broker account's rights. + +Relevant files: + +- `pkg/broker/install/sql/procedures/05_broker_add_job.sql` +- `pkg/broker/install/sql/procedures/02_broker_run.sql` + +Required remediation: + +1. Define the broker security model and the trusted roles allowed to enqueue work. +2. Revoke default `PUBLIC` access to broker functions and grant only the minimum required permissions. +3. Use a least-privileged broker role. +4. Consider a constrained job model rather than accepting arbitrary SQL text. + +### Critical: the broker is not PostgreSQL Row-Level Security aware + +The broker has no tenant or execution-principal field on jobs, no RLS policy design, and does not apply `run_as` when executing a job. Queued SQL consequently executes in the broker session's effective role. If that role owns application tables, has `BYPASSRLS`, or is otherwise exempt from policies, it can bypass tenant isolation. Conversely, simply enabling RLS can cause workers to see no eligible rows or fail unexpectedly because the worker has no policy context. + +Relevant files: + +- `pkg/broker/install/sql/tables/03_broker_jobs.sql` +- `pkg/broker/install/sql/procedures/02_broker_run.sql` +- `pkg/broker/install/sql/procedures/03_broker_set.sql` + +Required remediation: + +1. Define a tenant and execution-principal model before enabling RLS. Add an immutable tenant/principal reference to jobs and schedules; do not infer it from a mutable job name or SQL string. +2. Separate roles: a non-login schema/migration owner, a least-privileged broker runtime role that does not own tenant tables and does not have `BYPASSRLS`, and narrowly scoped application/enqueuer roles. +3. Enable and force RLS on tenant-owned application tables, with policies based on a transaction-local, validated tenant context or a trusted execution role. Account explicitly for PostgreSQL table-owner and `BYPASSRLS` exemptions. +4. Do not allow arbitrary queued SQL to choose its own identity through `SET ROLE`, `SET SESSION AUTHORIZATION`, or unvalidated session settings. Prefer approved job types/procedures with explicit grants; if impersonation is required, map an approved tenant identity to a controlled execution context in the same transaction. +5. Decide whether broker metadata is global operational data or tenant data. If tenant-scoped, apply policies to `broker_jobs` and `broker_schedule` too, while preserving a deliberate worker claim path. If global, revoke direct tenant access rather than relying on RLS. +6. Add integration tests using at least two tenant roles that prove cross-tenant reads, writes, and queued execution are denied, including scheduled work and retry/recovery paths. + +### High: single-instance protection is unsafe + +On registration response `retval = 3` (another broker is active), the application retrieves the active instance ID and continues to start workers. This contradicts the documented one-instance-per-database guarantee and can result in multiple broker processes operating under one instance record. + +The SQL registration check is also race-prone: it counts active rows without a uniqueness constraint or advisory lock, so concurrent registrations can both pass the check. + +Relevant files: + +- `pkg/broker/database_instance.go` +- `pkg/broker/install/sql/procedures/04_broker_register_instance.sql` + +Required remediation: + +1. Treat an existing active instance as a startup failure; do not reuse its ID. +2. Enforce ownership atomically with a database constraint and/or transaction-scoped advisory lock. +3. Define stale-instance takeover rules using heartbeat expiry and an ownership token. + +### High: dependency semantics are incorrect + +Dependencies are treated as satisfied whenever they are not pending. A dependency that is running, failed, or cancelled therefore permits execution. Dependencies also use non-unique job names, making the dependency relationship ambiguous. + +Relevant file: `pkg/broker/install/sql/procedures/01_broker_get.sql` + +Required remediation: + +1. Model dependencies by job IDs in a join table. +2. Permit execution only when every dependency has completed successfully. +3. Define the terminal behavior for failed or cancelled dependencies. + +### High: installer is not rerunnable + +Tables use `IF NOT EXISTS`, but trigger creation does not. Re-running `install` against an existing schema fails when creating existing triggers. Installation has no schema migration/version tracking and is not atomic across the whole schema. + +Relevant files: + +- `pkg/broker/install/sql/tables/02_broker_schedule.sql` +- `pkg/broker/install/sql/tables/03_broker_jobs.sql` +- `pkg/broker/install/install.go` + +Required remediation: + +1. Make trigger creation idempotent or explicitly replace/recreate triggers safely. +2. Introduce a schema-version table and ordered migrations. +3. Make each migration transactional where PostgreSQL permits it. +4. Add a test that runs installation twice and verifies the result. + +### Medium: advertised features are incomplete + +Schedules exist only as a table; no scheduler implementation creates jobs from cron expressions. `run_as`, worker idle timeout, and notification retry settings are configured or stored but unused. Queue creation also hardcodes one worker per queue. + +Relevant files: + +- `pkg/broker/database_instance.go` +- `pkg/broker/config/config.go` +- `pkg/broker/install/sql/tables/02_broker_schedule.sql` + +Required remediation: either implement these capabilities and test them, or remove them from the public documentation and configuration until supported. + +### Medium: startup failure leaks resources and active state + +If registration succeeds but queue or listener startup fails, the database instance has not yet been appended to the broker's managed instance list. Cleanup therefore does not stop its partially started resources, close its connection, or mark its database registration as shut down. + +Relevant file: `pkg/broker/broker.go` + +Required remediation: make startup transactional: on every failure after connection, stop started queues, close the adapter, and release or mark the instance registration before returning the error. + +### Medium: configuration permits broken runtime modes + +Configuration validation does not reject zero or negative queue, fetch, and timer values. These can leave jobs unprocessed or cause a zero-duration timer loop. TLS defaults to `disable`. + +Relevant file: `pkg/broker/config/config.go` + +Required remediation: validate all runtime limits as positive, set safe production TLS defaults, and require an explicit opt-out for plaintext local connections. + +### Medium: release and documentation drift + +The release workflow injects `main.version`, while the program exposes `main.Version`; release binaries will keep reporting `dev`. The Makefile build time is a fixed literal timestamp. README examples include an obsolete constructor signature, refer to a missing `examples/` directory, reference a nonexistent `make test` target, and show an outdated `broker_add_job` parameter list. + +Relevant files: + +- `.github/workflows/release.yml` +- `Makefile` +- `README.md` + +Required remediation: align release linker flags with exported variable names, generate build time dynamically, and make documentation executable/validated in CI. + +## Validation performed + +- `go vet ./...` passed. +- `go build ./cmd/broker` passed. +- Race-enabled tests for `./pkg/...` passed, but these packages contain no tests. +- The integration suite could not reach its PostgreSQL test database at `localhost:5433` from the audit sandbox because local socket access is blocked. Runtime integration behavior is therefore unverified in this audit environment. + +## Recommended next milestone + +1. Lock down execution permissions and the job security model. +2. Implement atomic exclusive-instance ownership and stale-owner recovery. +3. Add idempotent, versioned schema migrations. +4. Add unit and integration coverage for authorization, duplicate starts, crash recovery, dependency outcomes, notification loss, and repeat installation. +5. Reconcile implemented behavior, configuration, release metadata, and documentation.