Repository navigation
Harden the system initiator: reset links, an engine allow-list and refusal tests - #2617
Merged
Merged
Conversation
3 tasks done
pournasserian
added a commit
that referenced
this pull request
Oct 19, 2026
pournasserian
added a commit
that referenced
this pull request
Oct 19, 2026
Closes #2618 ## What The recurring retention purge of stored prompts (AUD-04), from the post-merge review of #2545: until now nothing purged them unless a person called `IModelCallLog.PurgeAsync`, and `SystemInitiator.Retention` was unused. - **`retention.purge` job** (`RetentionPurgeJob`, Core): - It runs as `SystemInitiator.Retention` and is listed for it in `EngineJobs` (#2617). - It runs only as the retention engine; a run as anyone else fails and purges nothing. - It first schedules its next run a day later, so a run that fails or dies after that still leaves one coming. A run whose every attempt fails before its scheduling commits leaves none until a host restarts; #2652 follows that up. - It then calls `PurgeAsync` batch after batch until a batch clears nothing, each batch in a scope of its own so a long backlog does not pile up in one context, reporting progress after each. - Each cleared call keeps its `model_call.purged` event, logged as the retention engine. A purge the engine may not make fails the run with its reason. - **A run is always scheduled** (`IRetentionSchedule`): - When a host starts, after the database is migrated, it enqueues a run as the retention engine if none is queued (`RetentionStartup`). A database the host cannot reach yet only delays that to the next start. - A run enqueues its successor before it purges, unless another run is queued. So a running run always leaves one queued, and the schedule is never empty. - `PurgeAsync`'s comment says it clears a batch at a time. - CONTRIBUTING.md: the retention purge. ## Why AUD-04 sets a retention for stored prompts and responses. A retention that a person must remember to apply is not enforced, so the product applies it itself, as the engine ADR-0012 named for it. Choices worth a look: - **Daily, not configurable.** It is well inside any retention of days, and a setting can come when someone needs one. - **Scheduled from the job and from startup**, rather than a timer in the host, so the schedule lives in the job queue (ADR-0010) and survives restarts, and any host can run the purge. - **Enqueued as the retention engine** through a scope only Core opens, so the job records the engine as the one that asked for it. - **Never empty rather than exactly one.** Without a unique slot in the job queue, two runs that start together can each see the other and must not both skip, or the schedule ends. Both enqueue, the purge is safe to repeat, and the next day's runs fold the two chains back into one. - **Builds on #2617** for `EngineJobs`, now in v2. ## Tests Integration, on SQLite and PostgreSQL where a database is involved: - A started host's worker runs the purge as the retention engine. The expired call is cleared, `model_call.purged` names the engine, and one run is left queued a day later. A second host that starts then enqueues none. The test waits for the run to succeed, and the first host's own run ends before the expired call goes in. - A run purges 1,101 calls past the retention across two batches, reporting 1,000 then 1,101. It logs `model_call.purged` for each as the retention engine, and schedules the next run a day later as that engine. - A run as the retention engine that finds one already queued schedules none. - A purge that fails fails the run with its reason, and the next run is still scheduled. - A run made as another engine fails with `model_purge_people_only`, and purges and schedules nothing. - Two runs at once leave at least one run queued. - `EngineJobs` lists the purge for the retention engine (unit). The full suite ran locally against a `postgres:17` container as well as SQLite, and markdownlint passes. No new package and no migration. After v2's webhook job, the model test scenario gives the job handlers a secret protector stub, since the retention tests resolve them together as the worker does. A started host now runs a retention job at once, so a test that reads the SQLite file while its host runs, as the webhook tests do, reads it without asking for sole access. ## Checklist - [x] Linked to an issue a maintainer has confirmed - [x] Targets `v2` - [x] Tests or docs updated where behaviour changed
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.
Closes #2600
What
Hardening from the review of #2558 (
SystemInitiator, ADR-0012): low-severity gaps, none exploitable today, that future engine or plugin code could have used to act beyond what ADR-0012 allows.IdentityPasswordReset.IssueAsync): an agent or an engine is refused withuser_people_only, as managing users already is. No token is issued and nothing is logged.SystemInitiator's constructor is internal to Core. Code gets an engine from the named ones (Checks,Retention).InitiatorKinds.RestoreSystemreads a stored row back only as one of the product's engines, with its own id; any other component name is refused as a fault.EngineJobsin Core lists each job handler that may run as an engine, with that engine. Today the check run is listed aschecks.job_engine_not_allowed, without running it, when:Why
The review of #2558 found three gaps:
IssueAsyncdid not check its caller.SystemInitiatoritsRunAsreturned, and anyone could make one.Its first review here found two more: code running as an engine could pass the engine on through a job it enqueued, and reading a row could make up an engine. Nothing exploits these today, since only Core code runs, but ADR-0012's "no caller can borrow an engine's permissions" should hold for the code that comes later, plugins included.
Choices worth a look:
CheckRunJob, so it cannot pass as it. Code in the same process could still get around this through reflection; the allow-list stops a handler from claiming an engine, not code set on misuse. Per-plugin permissions come with the plugin SDK (EXT-03, M3).OnBehalfOffrom Harden the instruction check and the model gateway's spend cap #2586.Tests
Unit:
EngineJobs:checksSystemInitiator: one stable id per component, read back only for the product's engines; no public constructor.Integration, on SQLite and PostgreSQL:
checksfails its job withjob_engine_not_allowedand never runs.ChangeSetChecksTests).The full suite ran locally against a
postgres:17container as well as SQLite, and markdownlint passes. No new package and no migration.Checklist
v2