Skip to content

feat(session-store): file-backed approval store - #635

Closed
Shearerbeard wants to merge 20 commits into
mainfrom
mshearer/file-approval-store
Closed

feat(session-store): file-backed approval store#635
Shearerbeard wants to merge 20 commits into
mainfrom
mshearer/file-approval-store

Conversation

@Shearerbeard

Copy link
Copy Markdown
Collaborator

What

A file-backed ApprovalStore backend: one JSON file per decision id ({root}/tickets/{id}.json, {root}/decisions/{id}.json), reusing ParkedApprovalRecord/DecisionRecord as the on-disk shape. Selected by AURA_SESSION_STORE=file with a required AURA_SESSION_STORE_PATH; the A2A task store and event bus stay in memory (single-pod, single-writer model).

Contract (what park mode keys on)

  • resolve refuses past the ticket's expires_at, uniformly with an unknown id - enforced on BOTH the file and memory backends, so the contract holds on whichever is configured.
  • get returns the ticket before and after the decision, and decision returns the recorded decision, both until remove (retention until removed, not a TTL margin).
  • resolve moves the ticket into the decision file rather than deleting it; at-most-once via File::create_new on the decision file (AlreadyExists reads as NotFound).
  • cancel_request removes undecided tickets by owner id; decision ids are validated as canonical UUIDs at every path-building site, so nothing escapes the store root.
  • Ticket publication is temp-file + same-directory rename; the create_new-to-sync crash window is fail-closed and documented (module doc) with a one-file operator recovery.

Testing

  • The backend-agnostic approval battery is factored out of the redis integration test into tests/common and run against BOTH the file and memory backends without Docker, plus retention, expiry, resolve-moves-ticket, cancel-by-owner, and restart-durability cases (register, drop the store, reopen, resolve, read the decision).
  • cargo test -p aura -p aura-web-server and cargo clippy --workspace --all-targets -- -D warnings clean; live restart check passed (register, kill -9, restart, resolve 204, decision file on disk).

Review trail

Two fresh-context reviews returned explicit PASS: a granular Rust review (at-most-once, expiry on both backends, path containment) and a feature-scope frontier review (poll-shape coherence with the planned run-resource resume UX, durability boundary). The frontier round's three doc-coherence findings were fixed in e3195f45 and re-reviewed clean. Full findings ledgers live on the board cards.

Adds scripted streams to MockAgent so tool lifecycle events can be
driven without a provider or MCP server. Moves nine aura.tool_*
assertions into streaming::handlers, and adds final-chunk usage and
stream-shape tests. The progress-token and FIFO-ordering tests stay
in the integration suite, where a real provider is required.

Signed-off-by: Jacob Hull <jacob@planethull.com>
@Shearerbeard
Shearerbeard requested a review from a team September 2, 2026 13:52
@greptile-apps

greptile-apps Bot commented Sep 2, 2026

Copy link
Copy Markdown

Greptile Summary

The PR adds an environment-selected, file-backed approval store that persists tickets and decisions across process restarts while retaining in-memory task and event capabilities.

  • Adds file-backend configuration and web-server factory wiring.
  • Implements per-decision JSON persistence, expiry checks, resolution claims, cancellation, and removal.
  • Expands backend-agnostic conformance and restart-durability tests.

Confidence Score: 4/5

The PR should not merge until file-backed resolution cannot return an error after durably applying the decision; the remaining executor-blocking and documentation issues are non-blocking.

A ticket-deletion error after the decision file is synced makes the HTTP result disagree with the authoritative stored decision, allowing the protected operation to continue despite the approver receiving an error.

Files Needing Attention: crates/aura/src/session_store/file.rs, crates/aura-config/src/session_store.rs

Important Files Changed

Filename Overview
crates/aura/src/session_store/file.rs Implements the durable approval backend, but resolution can report failure after committing a decision and its synchronous filesystem work can block executor threads.
crates/aura/src/session_store/memory.rs Adds expiry refusal to the memory backend while preserving its existing at-most-once resolution behavior.
crates/aura-config/src/session_store.rs Adds validated file-backend environment configuration; some behavioral documentation is attached to data definitions.
crates/aura-web-server/src/session_store.rs Wires the file approval store into the backend factory while intentionally retaining in-memory task and event implementations.
crates/aura-web-server/tests/file_session_store_test.rs Covers conformance, retention, expiry, cancellation, on-disk layout, and restart durability, but not post-commit ticket-deletion failures.

Sequence Diagram

sequenceDiagram
    participant Client as Approver client
    participant Registry as Pending approvals
    participant Store as File approval store
    Client->>Registry: Resolve decision
    Registry->>Store: resolve(id, decision)
    Store->>Store: Read ticket and check expiry
    Store->>Store: create_new decision file
    Store->>Store: Write and sync decision
    Store->>Store: Remove ticket file
    Registry->>Registry: Publish wake event
    Registry-->>Client: Accepted
    Note over Registry,Store: Polling the decision file is the lost-publish fallback
Loading

Reviews (1): Last reviewed commit: "docs(session-store): name the resolve cr..." | Re-trigger Greptile

Comment thread crates/aura/src/session_store/file.rs Outdated
Comment thread crates/aura/src/session_store/file.rs
Comment thread crates/aura-config/src/session_store.rs
teriyakichild and others added 19 commits September 2, 2026 10:05
Pin rig-core to the fork rev that adds prompt-cache support and
cache-usage reporting (mezmo/rig#12), and switch rig-bedrock from
crates.io =0.3.10 to the fork's vendored copy at the same rev — it
carries the cachePoint request support and builds against the fork's
rig-core, removing the standing semver hazard with crates.io 0.3.11+.
Exclude ./rig from the workspace so a local checkout can be
path-pinned during fork work without cargo resolving its crates
against this workspace root.

Refs: #630
Signed-off-by: Tony Rogers <tony@tonyrogers.me>
Opt-in per agent because caching changes billing (cache writes bill at
a premium) and, on Bedrock, sending cachePoint blocks to a model
without caching support fails the request outright. Workers inherit
the flag with [agent.llm] or override it per worker.

Refs: #630
Signed-off-by: Tony Rogers <tony@tonyrogers.me>
Wire the prompt_caching flag into the Anthropic and Bedrock completion
models at all three construction sites (single-agent builder,
orchestration coordinator, orchestration workers), and surface the
provider-reported cache split end to end:

- UsageState accumulates cache read/creation tokens across turns; the
  streaming hook captures them via the fork's cache_token_usage() and
  includes them in the usage log line.
- StreamItem::TurnUsage carries the per-turn cache split so the
  orchestration path accumulates it into the shared UsageState via
  TurnTally::record.
- aura.usage gains optional cache_read_input_tokens /
  cache_creation_input_tokens (omitted when no provider reported cache
  usage, so existing consumers see an unchanged payload). Both are
  sub-counts of prompt_tokens: providers that report input exclusive
  of cached tokens are folded in the fork, and OpenAI already includes
  them.

Verified live against Anthropic-format mocks, an OpenAI-format mock,
and real Bedrock (us.anthropic.claude-sonnet-5): a cold request
reports the full prefix as cache creation, a warm one reports it as
cache read, and prompt_tokens stays consistent with the provider
total.

Refs: #630
Signed-off-by: Tony Rogers <tony@tonyrogers.me>
The stream handler passes the aura.usage cache split into the REPL:
the status line's tokens segment renders "in N (M cached) / out K"
once any turn reports cache reads, and DisplayEvent::Usage records the
split (optional fields, so old event logs still replay) so resumed
conversations rebuild the counter.

Refs: #630
Signed-off-by: Tony Rogers <tony@tonyrogers.me>
Translate the fork's gen_ai.usage.cache_read_input_tokens /
cache_creation_input_tokens span attributes to OpenInference
llm.token_count.prompt_details.cache_read / cache_write. The spec
defines prompt_details as sub-counts already included in
llm.token_count.prompt, which matches how the fork folds cache tokens
into input_tokens. Phoenix (v13+) renders these in the token breakdown
and prices them via its cost models.

Refs: #630
Signed-off-by: Tony Rogers <tony@tonyrogers.me>
The resume path treats the usage JSONL as the authoritative source for
conversation totals and seeds the status line from it after replay,
but cache-read tokens were rebuilt only from retained display events —
a conversation whose older events were discarded resumed with full
prompt/completion totals and an under-counted cached share. Store the
cache split on each usage entry (absent on pre-existing lines, which
count zero) and seed the cached counter from the same ledger.

Refs: #630
Signed-off-by: Tony Rogers <tony@tonyrogers.me>
The prompt_caching field comments narrated defaults and Bedrock's
rejection behavior, and TurnUsage narrated when its cache field is
populated — behavior owned by the wiring and mapping code. Trim the
type-level comments to what the values are and state the Bedrock
opt-in rationale at the builder branch that applies the flag.

Refs: #630
Signed-off-by: Tony Rogers <tony@tonyrogers.me>
The resume path seeded the status-line counters from the usage ledger,
but any later display-only replay (/expand, a style repaint, stream
toggles) reset them and rebuilt from the display-event log alone,
clobbering the authoritative totals with potentially truncated ones —
for prompt and completion as well as the new cached count. Move the
ledger seed into replay_event_log_global itself, keyed off the active
conversation dir, so the ledger gets the last word on every replay; an
empty ledger keeps the replay-derived values. The per-site seeds after
resume are gone — replay owns it.

Refs: #630
Signed-off-by: Tony Rogers <tony@tonyrogers.me>
The final aura.usage event mixed populations: prompt/completion came
from rig's turn-aggregated Final usage (which includes tool-only
turns), while the cache split came from the streaming hook, which rig
invokes only on turns that produced assistant text. On a tool loop the
cache counts could under-report relative to the totals beside them,
breaking the sub-count contract.

Carry rig's turn-aggregated cache split through FinalResponseInfo into
TurnState, and resolve it in resolve_billed_usage from the same source
as the totals: the aggregated Final when present, the hook counters
(which see every turn via TurnUsage) on the orchestration fallback.
Regression test covers the tool-only-turn population mismatch.

Refs: #630
Signed-off-by: Tony Rogers <tony@tonyrogers.me>
Add the prompt_caching flag to examples/reference.toml for anthropic
(cache_control, billing tradeoff, worker inheritance) and bedrock
(cachePoint placement, supported-model constraint), and note that
OpenAI caches automatically with reads visible in aura.usage. Also
update the resume_conversation doc for the three-element usage tuple
and extend the rig-bedrock pin comment to cover the reused 0.3.10
version string and the rig-derive resolution.

Refs: #630
Signed-off-by: Tony Rogers <tony@tonyrogers.me>
A replay triggered while a response is streaming (/expand, /help,
/conversations, /model, /style) reset the status-line counters and
rebuilt them from the global display log plus the usage ledger. The
in-flight turn's usage is buffered per-turn and reaches both of those
only at turn end, so a replay landing after the turn's usage event had
already been added live would erase that turn's prompt, completion,
and cached-token counts — and since the counters accumulate deltas,
nothing re-added the lost turn afterward.

Skip the counter reset, the per-event rebuild, and the ledger seed
whenever a turn is in flight: the live counters are authoritative for
the whole PROCESSING window, and mid-stream replays only need to
repaint the transcript. Idle replays (resume, prompt-time /expand and
style changes) rebuild exactly as before. The same guard covers the
context-occupancy and scratchpad counters, which had the same
clobbering exposure.

Refs: #630
Signed-off-by: Tony Rogers <tony@tonyrogers.me>
The per-attempt tool-calls.json files duplicated every tool output in
full: each record embedded the complete clean output, and every append
re-read, re-parsed, and rewrote the whole file, so bytes written grew
quadratically with call count and multi-MB outputs went through JSON
string escaping on every subsequent call in the attempt. All of that
happened while holding the shared persistence mutex, blocking every
other worker's completion hook. OTel spans now cover the debugging
role these files served.

The two runtime consumers only ever read the condensed form (tool
name, reasoning, duration, outcome byte count, artifact filename), so
record ToolTraceEntry directly into an in-memory per-task map on
ExecutionPersistence. Continuation-prompt rendering and the run
manifest's tool_trace read from that map; full outputs remain
available via promoted artifacts and OTel. ToolCallRecord and its
append/load paths are gone.

Refs: #636
Refs: #637
Signed-off-by: Tony Rogers <tony@tonyrogers.me>
The AURA_PROMPT_JOURNAL-gated prompt-journal.md predates OTel support.
With content recording (OTEL_RECORD_CONTENT), every coordinator and
worker prompt is captured on the agent.stream/agent.turn spans, so the
journal duplicates what tracing already provides. The per-worker
prompt.txt files written by execution persistence also remain.

Removes the module, the env flag, and the orchestrator's
current_iteration atomic, whose only reader was the journal.

Fixes: #637
Signed-off-by: Tony Rogers <tony@tonyrogers.me>
The lib test target stopped compiling when FinalResponseInfo gained
the cache_usage field: the streaming handler tests' final_item helper
still built the struct without it. The tests exercise the aggregated
usage path, so a bare None matches what they simulated before.

Refs: #630
Signed-off-by: Tony Rogers <tony@tonyrogers.me>
FileApprovalStore implements the six-method ApprovalStore trait over
one JSON file per decision id, reusing ParkedApprovalRecord and
DecisionRecord as the on-disk shape (tickets/ and decisions/ under the
configured root, temp-file plus same-directory rename publish).
resolve refuses past expires_at uniformly with an unknown id, claims
at-most-once via create_new on the decision file (the envelope carries
the moved ticket so get recovers it until remove), and cancel_request
scans undecided tickets by owner id. Ids are validated in canonical
UUID form at every path-building site.

The same expiry refusal lands on the memory backend so the contract
holds on whichever backend is configured. aura-config gains the file
backend (AURA_SESSION_STORE=file with required
AURA_SESSION_STORE_PATH) and the web-server factory arm keeps tasks
and the event bus in memory. The backend-agnostic approval battery is
factored into tests/common and run against both the file and memory
backends without Docker, plus the retention, expiry, and restart
durability cases; the redis integration test keeps its Docker-gated
cases and wires to the shared battery.

Card: P47
Frontier round-1 findings (all MINOR): the module doc now records the
create_new-to-sync crash window, its fail-closed aftermath, and the
one-file operator recovery, so decision() consumers treat Err(Decode)
on a known id as that recoverable state; the lone sync_all is
commented as best-effort beyond the process-restart boundary instead
of reading as a host-crash guarantee; and the ApprovalStore::resolve
trait doc now describes the move-and-retain semantics instead of the
delete-on-resolve shape the file backend rules out.

Card: P47
…olve at the sync

FileApprovalStore becomes a Clone handle over an Arc<Inner> holding the
store root and the single-writer mutex. Each ApprovalStore trait method
is now a thin wrapper that moves owned copies of its arguments into a
spawn_blocking closure running the verbatim sync body (register_sync,
get_sync, resolve_sync, decision_sync, remove_sync, cancel_request_sync);
a JoinError maps to the store request error (ResolveError::Store for
resolve), which callers already treat fail-closed. The std Mutex is taken
only inside the sync methods, so no guard can cross an await. tokio::fs
and an async mutex were ruled out: tokio::fs is one blocking hop per
syscall, and resolve's compound read-claim-write would need an async
mutex held across awaits.

resolve's ticket removal becomes best-effort past the commit point: the
write_all+sync_all is the commit, and a failed remove_file logs a warning
naming the decision id and leaves the stale ticket, which is benign --
get returns the identical record from either file, a repeat resolve
still fails closed on the create_new claim, and remove/cancel_request
clean the stale file up.

New conformance test: with a read-only tickets directory, resolve still
returns Ok, the decision reads back, get still returns the ticket
record, and a repeat resolve is NotFound. The test skips when the
process bypasses directory permissions (root), reusing the workspace's
empirical write-probe pattern.

Signed-off-by: Mike Shearer <shearerbeard@gmail.com>
Card: P47
…t conventions

Per CLAUDE.md's code comment conventions (document behavior where it
lives; the drift test; red-flag words on type definitions), behavior
narration moves off data and onto the code that implements it.

aura-config: the "created when missing" narration leaves the path field
and FileSessionStoreConfig::from_env -- FileApprovalStore::open already
documents directory creation at the implementing site, so the restatements
are deleted rather than moved. The enum variants keep only what each
value is; the "tasks and bus stay in memory" deployment clause moves to
the site that composes the backend.

aura-web-server: the backend structs keep only what they are; the file
factory arm carries the composition note it owns (file-backed approvals,
in-memory task store and bus -- the single-pod deployment model). The
in-memory struct's process-local narration restated the in-memory
module's own doc and is deleted. FileSessionStore::new drops its
directory-creation restatement of FileApprovalStore::open.

file.rs: the lock-field comment is gone (the module doc carries the
mutex contract), and the method-body comments that restate the
module-doc contract list are cut -- the retention note in get, the
create_new claim and expiry notes in resolve, and the undecided-only
note in cancel_request. The module doc stays the single contract home;
genuinely local why-comments (the claim undo on write failure, the
temp-file skip in cancel_request) are kept.

Signed-off-by: Mike Shearer <shearerbeard@gmail.com>
Card: P47
Per the maintainer's review ruling on the store PR, the comment pass
cuts what restated the module doc, narrated behavior from data, or
hedged past the point it made. The module doc keeps every contract
fact (layout, the four contract bullets, path safety, blocking-pool
semantics including non-cancellation, crash window and recovery) in
roughly half the lines.

Signed-off-by: Mike Shearer <shearerbeard@gmail.com>
Card: P47
@Shearerbeard

Copy link
Copy Markdown
Collaborator Author

Closing in favor of #643: nightly is now the primary PR target, and the retarget path causes odd GitHub behavior, so the store ships as a fresh PR against nightly carrying the v2 fix round (blocking-pool restructure, resolve commit-point semantics, comment pass).

@github-actions github-actions Bot locked and limited conversation to collaborators Sep 2, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants