feat(core)!: the store assigns seq and ts - #154
Merged
Conversation
tests/core/conftest.py built every example event's seq from its position in PAYLOADS, so the snapshots pinned a number that had nothing to do with what they exist to pin. Adding a kind was therefore order-sensitive: appending was only safe if nobody else appended, and #112 and #116 each appended two payloads at once, so both shipped snapshots claiming seq 17 and 18 and whichever merged second had to regenerate files whose only diff was a seq bump. Every example now carries seq 0. These snapshots pin one thing — how each payload kind serializes — and a per-kind position in that tuple was never part of it. Nothing depended on the numbers being distinct: every seq-sensitive test stamps its own through make_event, and _run re-enumerates from zero. Snapshots regenerated deliberately via `make golden`. 20 of 21 files change and every diff is the seq line alone, verified — run.started was already 0. This is not a schema change; no payload, field or serialized shape moved. Closes #121 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ot the timing The trial ended by counting how many of its 10 runs had the two processes' appends alternate in the file, and asserting the count was non-zero. That is a fact about the machine, not a promise of the store: peers released from a barrier onto a loaded or single-usable-core runner can serialise across every trial. It went red on CI for exactly that reason, on a branch whose diff could not reach the code under test, and it does not reproduce locally even pinned with taskset -c 0 — the signature of a timing assertion rather than a defect. Same treatment #113 gave the resume race's `raced` counter in this file: the count is printed, not asserted. That leaves the file fully converted; a sweep for the remaining pattern ("never overlapped", "did not race", assert raced / interleaved / overlap) across tests/ finds nothing else. Every per-trial assertion is untouched and still carries the invariant: each run's kinds in order, each run contiguous from 0 and closed by exactly one terminal event, one event per (run_id, seq), and the worker's own spent-seq refusal failing the trial while the peer is still writing. Verified: passes normally (4/10 interleaved), passes under `taskset -c 0` (5/10), and goes red on a planted duplicate (run_id, seq) — "a seq answers to two events". ADR-D11 §6's concurrency case lands with the port change, where the API it tests exists; writing it here against a signature that does not yet exist would be noise rather than evidence. Closes #127 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ADR-D11 §5, as amended by #153. Callers hand over payloads and get finished events back; the store assigns seq and ts inside whatever makes its write indivisible. A number allocated and persisted together cannot be allocated and not persisted, which is what makes seq dense enough to be the loss check it is documented to be. append(log_key, payloads, ctx, origin) -> list[Event] claim_start(log_key, opening, ctx, origin, stale_after) -> (SessionClaim, Event | None) claim_resume(log_key, run_id, resumed, ctx, origin) -> Event | None last_seq removed Port goes 8 methods to 7. read, read_run, list_runs and run_status are untouched. append stays ctx-derived, with no override parameter: a caller that could address any run could file an event under a run it is not playing. The one write belonging to a different run — the terminal event a takeover stamps for a run it stepped over — passes a ctx built for that run from the event SessionClaim now hands back. SessionClaim.overridden carries each abandoned run's last event rather than its id, which every store already holds, having compared that event's ts to decide the run was stale. stale_before: datetime becomes stale_after: timedelta. The caller no longer owns the clock the comparison is made in, so it cannot compute a cutoff in it. claim_resume keeps its status condition verbatim and loses the stale-seq half, which existed only because the caller stamped before claiming. Its docstring now says what that check never covered and still does not: nothing names which interrupt a resume answers. That is #94's, in a schema PR. Deliberately red: this changes the ABC, so all four adapters and the Runtime stop satisfying it until their own commits land. Committed alone so the contract is reviewable on its own. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
First adapter under the new port, and the reference the other three get diffed against. _stamp is the whole mechanism: read the run's last seq, build the events, extend the log — plain dict work with no suspension point anywhere in between. Both claims go through it rather than through append, because an await between reading the last seq and extending is all it would take for two tasks to be handed the same number. append's own asyncio.sleep(0) (issue #87's fidelity yield) stays after the mutation, where it opens no window. _refuse_a_taken_seq is deleted. It was the durable stores' unique index rendered in a dict, and it defended against a caller supplying a seq that was already spent. No caller supplies a seq now, so two events at one seq is unconstructible rather than merely refused. claim_start returns the abandoned runs' last events rather than their ids — it was already holding each one to compare its ts, and now hands it back instead of discarding it. It also does its own now - stale_after, since the clock moved here. claim_resume keeps the status condition and drops the seq check. Its run_id/tenant guards collapse into one ctx check: the envelope is built from ctx, so an event for the wrong run or tenant can no longer be handed in. Measured on this store: claim on an idle session -> seq 0, ts set, origin stamped two more payloads -> seqs [1, 2] second turn, live session-> refused, held_by=r1, no event 20 concurrent appends -> seqs [0..19], no duplicates <- ADR-D11 §6 double resume -> first returns seq 2, second returns None Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
append now takes BEGIN IMMEDIATE, which it never did. Reading MAX(seq) inside the deferred transaction it used to run and then inserting upgrades read→write mid-transaction, and SQLite answers that with SQLITE_BUSY_SNAPSHOT — which does not honour busy_timeout (#84), so a peer committing in between would be an error rather than a wait. Taking the write lock first makes the read and the insert one step, which is the decision. ADR-D11 §6's row said this transaction already existed; it did not, and #153 corrected the ADR to say so. _stamp_and_insert is the shared assign-build-insert, callable only with the lock held; all three write paths go through it. ts comes from SQLite, not this process, so N workers on one file compare one clock. Read as strftime('%Y-%m-%dT%H:%M:%f+00:00', 'now') rather than CURRENT_TIMESTAMP, which is whole seconds and would give every event in a busy second the same ts — visible coarsening on the wire for nothing. One ts per call: a batch is a single indivisible write, so it happened at one instant. The UNIQUE (tenant, log_key, run_id, seq) index stays, and its comment now holds in the way the ADR describes — it stopped being the guard against a caller reusing a seq (there is no such caller) and became the proof that assignment is correct. Same five behaviours as the memory store, measured on this one: claim on an idle session -> seq 0, ts set, origin stamped two more payloads -> seqs [1, 2] second turn, live session-> refused, held_by=r1, no event 20 concurrent appends -> seqs [0..19], no duplicates double resume -> first returns seq 2, second returns None Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Same shape as sqlite, over the per-log advisory lock this store already took on every write. That lock existed for the paging guarantee — BIGSERIAL is assigned at insert and published at commit, so an unlocked append can be given a later id than an in-flight claim and still commit first — and it is now also what makes reading this run's last seq and inserting the next one indivisible. ts is clock_timestamp(), not now(). now() is the transaction's start time, so every event in a batch would carry the timestamp of the statement that opened it — and on the claim path, the statement that set lock_timeout. Reading Postgres's clock rather than this process's is the point: N workers sharing one database compare one clock, which is what stale_after needs to mean anything (ADR-D11 §4). Verified by ruff and ty here; the behaviour is proven by the shared contract suite against a real server in CI, which is where this store's guarantees have always been measured (#75). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The next seq comes off the run's existing ZSET index — read under WATCH, so a peer spending the same number between the read and EXEC aborts this write and _watched retries on what the peer actually wrote. ADR-D11 §6 originally prescribed INCR on that key, which would have returned WRONGTYPE on every append; #153 corrected the table and this implements the corrected row. _refuse_a_taken_seq is deleted. It existed to catch a caller reusing a seq, and there is no such caller now; the WATCH that used to protect the check is what protects the assignment instead. Its StoreError-on-duplicate arm goes with it — a peer racing for the same number now loses an EXEC and retries, which is a race somebody lost rather than the corruption that error reported. claim_resume no longer watches the seq key itself: _stamp does it, and the status key remains this claim's own condition. The clock is an injected callable, not Redis's TIME. Per ADR-D11 §4 the SQL stores read the backend's clock because their atomicity rests on a transaction they are already inside; here it does not — the WATCH is the mechanism — and a TIME round trip on the hot path would buy nothing. Measured against a real Redis 7 (the running agentdeck-test-redis, db 15): claim on an idle session -> seq 0, ts set, origin stamped two more payloads -> seqs [1, 2] second turn, live session-> refused, held_by=r1, no event 20 concurrent appends -> seqs [0..19], no duplicates double resume -> first returns seq 2, second returns None The same five, run against a real Postgres 16, pass too — so the previous commit's "proven in CI" is now "measured here as well". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
count() is gone from run() and from the resume claim, and the Iterator[int] that was threaded through seven signatures goes with it. _stamp is deleted; _record is now append-and-fan-out, and the store returns the finished event. Three call sites the ADR originally called "seq arithmetic that stops existing" needed three different answers, which is what #149 found and #153 corrected: - the arithmetic one (the resume claim's max(seq) recovery) simply goes; the store assigns from the run's own log, so seq still continues across a process restart with no counter here to recover. - the cancellation-path probe asked last_seq(...) >= 0 to find out whether the claim committed before the client disconnected. It now asks run_status(...) is not PENDING, which answers the same question: PENDING means no lifecycle event, indistinguishable from a run the store never saw, and every run records run.started first. - _close_abandoned read the run's tail for the envelope fields its closing event had to inherit. It now receives that event from SessionClaim, which the store had already read to decide the run was stale. _close_abandoned is where the shape earns itself. It writes into a *foreign* run, and rather than an override parameter it builds that run's own RunContext — replace(ctx, run_id=..., session_id=...) — and passes the abandoned run's origin. append stays ctx-derived, so no caller can file an event under a run it is not playing. ty passes across the package. The test suite does not: 190 tests across 12 files construct stamped events and call the old signatures, the 823-line store contract suite most of all. Those are the next commits. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…he seqs it assigned Every case here used to hand the store a pre-stamped `Event` built by a `_event(seq, ...)` helper, which is now unconstructible: `append`, `claim_start` and `claim_resume` take payloads and derive the whole envelope from the `RunContext`. So the helper is gone and its two jobs split — `_ctx(tenant, run_id, log_key)` says who the write belongs to, and the payload constructors say what happened. A case that writes for a second run passes a context built for it, not a `run_id=` override on an event nobody constructs. `stale_before: datetime` became `stale_after: timedelta`, and the store owns the clock, so a case can no longer backdate an event. Most of them do not need to: `NOTHING_IS_STALE` and `EVERYTHING_IS_STALE` are a window nothing in the test can fall outside or inside, and `timedelta(0)` is stale in the correct direction on all four backends because equality fails the `>` comparison. The two cases that need one run stale *beside* a live one arrange a real age gap with a sleep — arranging age, not asserting timing. New, and the case the whole port change rests on: 20 tasks appending to one run at once must come back numbered 0..19 with no duplicates, on every parametrized backend. Both halves are asserted, the seqs the store handed each caller and the seqs the log reads back, because only the first catches a store that returns a number it never wrote. Measured against a store with one `await` between reading the run's last seq and extending the log: it hands 0 to all twenty, and every other case in this file still passes. Deleted rather than converted, because each one tests something no caller can now express: - `test_last_seq_is_negative_one_for_a_run_with_no_events`, `test_last_seq_tracks_the_highest_seq_appended_for_that_run`, `test_last_seq_is_scoped_to_one_run_not_the_whole_log`, and the `last_seq` params in `_SQLITE_CALLS` and `test_the_focused_queries_never_answer_from_another_tenants_log` — the method is off the port. The tenant-isolation case keeps its point through `read_run`. - `test_one_seq_per_run_is_refused_a_second_time`, `test_a_batch_holding_its_own_duplicate_seq_is_refused_whole`, `test_one_seq_per_run_holds_on_the_claim_paths_too` — a caller has no `seq` to spend twice, so the refusal is unconstructible rather than merely unenforced. What they protected is now the concurrency case above. - `test_a_claim_carrying_a_stale_seq_loses_even_though_the_run_is_waiting_again` — the stale-seq half of `claim_resume`'s contract existed because the caller stamped before claiming. Status is the whole condition now. - the `pytest.raises(ValueError, match=<tenant>)` halves of `test_claim_start_never_sees_another_tenants_open_run` and `test_claim_resume_never_reaches_into_another_tenants_waiting_run` — the tenant comes from the context, so there is no foreign one to refuse. Both keep their isolation half, and the claim_start case now asserts each tenant's log holds only its own events. Renamed: `test_a_claim_must_carry_an_event_for_the_run_it_names` reads `..._must_be_made_in_the_context_of_the_run_it_names`, since the mismatch it still catches is between `run_id` and `ctx.run_id`. `test_one_seq_per_run_does_not_stop_two_runs_sharing_a_seq_in_one_log` reads `test_every_run_in_one_log_counts_its_own_seq_from_zero` — same promise, now stated as what the store does rather than what it declines to refuse. Two cases added beside it for the numbering a caller can no longer check by construction: a batch is numbered in the order it was handed over, and a second batch carries on from where the first stopped. `NeverYields` follows the port to seven methods. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Same conversion as the contract suite, in the four files that keep the evidence one backend alone can be wrong about. Each `_event(seq, ...)` builder is gone; where a case needed several events it asks for several payloads in one append and reads the seqs back, which is now the only way to know what it was given. Both claim races are re-asserted through the return values rather than a bool: `claim_resume` hands back the event it wrote or `None`, and `claim_start` a `(SessionClaim, Event | None)` pair. The two session-claim races gained a line for the second half of that pair — exactly one peer is handed an event — because a store could name one holder correctly and still return the loser an event it never wrote. Deleted: - `test_an_event_stamped_for_another_tenant_is_refused`, from the memory and SQLite suites both. The tenant comes from the `RunContext` that chose the bucket, so there is no foreign tenant left to stamp and nothing to refuse. Tenant isolation keeps its own case in each file (two tenants under one session id read only their own events). - the `last_seq` params from the redis and postgres `_CALLS` boundary parametrizations, the method being off the port. Both lists still name every remaining method, which is what makes a method added later without the wrapper a missing case rather than a silent leak. - `test_a_colon_in_a_tenant_id_cannot_reach_into_another_tenants_log` asked `last_seq` for one of its four "the intruder sees nothing" probes; it asks `read_run` instead, so the case keeps all four. Postgres's stale-run case now asserts what `SessionClaim.overridden` actually carries — the abandoned run's own last event, compared against the event the seeding append handed back, rather than its id. That is the field `Runtime._close_abandoned` builds the closing event's envelope from, so an id would have passed while telling the caller nothing it needs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…seam moved to the store `check_contiguous(...) == [2]` becomes `== []`. That one line is what ADR-D11 exists for: a store that refuses a `status.reported` used to cost the run a `seq` as well as the report, because the number was taken before the write and stayed spent when the write failed. It is allocated inside the write now, so a refused append takes nothing, and a hole in a log can only mean an event was genuinely lost — which is what `service.py`'s own refetch promise always claimed. The comment inverts with it: a known consequence becomes something that can no longer happen. `Runtime(clock=...)` is inert. Every event's `ts` comes from the store, so the ~30 test call sites passing `clock=lambda: TS` were freezing nothing while reading as if they were, which is worse than not freezing at all. The kwarg is stripped from all of them, and the two cases that genuinely assert a timestamp now inject the clock where it lives: `MemoryEventStore(clock=lambda: TS)`. `test_the_envelope_timestamp_comes_from_the_injected_clock` is renamed `..._comes_from_the_stores_clock` and says which seam that is. The contract suite's `store` fixture takes the frozen clock for the same reason. The parameter itself is left on `Runtime.__init__` and `build_runtime`, documented as no longer stamping anything — removing a public keyword is a breaking change and not this PR's to take. Staleness is arranged through the store's clock too. `_abandoned` used to hand-stamp a `run.started` at `TS - 10min`; nothing can hand-stamp a `ts` now, so `_Held` is a clock a test moves by hand and `_leave_open` writes the abandoned run's opening with it held in the past, then returns it to `TS`. Same determinism, no sleeps, and it is closer to what the store actually sees. `_Ticking` moves from the Runtime to the store unchanged. Two test doubles were overriding methods that no longer exist in that shape, which would have made them silently stop intercepting: - `_SlowClaim.claim_start` took `(log_key, event, ctx, stale_before)` and gated on `event.run_id`; it now takes the port's five arguments, gates on `ctx.run_id` and returns the `(SessionClaim, Event | None)` pair. - `_CannotClose` refused `last_seq`, which is off the port. The takeover's closing write goes through `append` with the *abandoned* run's context, so it refuses that instead — the same failure at the same point, told apart from this turn's own appends by the one thing that distinguishes them. - `_RefusesReports.append` filtered `event.kind` over pre-stamped events; it filters `payload.kind` over payloads. Without this the store would have refused nothing and the `== []` above would have passed vacuously — the caplog assertion is what proves it still bites. **A behavior change ADR-D11 implies but never states**, so it is recorded here rather than buried: `test_a_run_that_writes_again_after_being_taken_over_fails_instead_of_reusing_a_seq` is now `..._lands_behind_its_terminal_event`. A run whose session was taken over prematurely used to die on its next write — it held a spent `seq` and the store refused it. It holds no number any more, so its writes are assigned the seqs *after* the takeover's `run.failed` and go through: the run plays to completion and the log ends `[run.started, text.delta, run.failed, run.completed]`. The damage is still bounded and still detectable, but by `check_terminal` reporting two terminal events rather than by a `StoreError` reaching the engine. The log stays dense and no `seq` answers to two events, both of which are structural now instead of enforced. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ds them
`_race_crossrun` self-stamped `enumerate(script)` and then called `_refuse_a_spent_seq`, which
required the log to refuse this run's `seq` 0 a second time. A peer holds no `seq` any more —
the store reads the run's last one under the file's write lock and assigns the next — so the
duplicate that assertion demanded is unconstructible rather than merely unenforced, and the
whole arm is deleted along with the `_stamped` builder behind it.
**This supersedes one line of `1e4df66`'s message**, which listed the spent-seq refusal among
the invariants that stay. It does not stay; it becomes unreachable.
What replaces it is the other side of the same promise, checked where it is cheapest to catch:
each peer asserts per write, while the other is still writing into the same file, that the
events it got back are one event at its own run's next number. A store that handed two writers
one number fails inside the worker, which the trial reads out as a peer that exited non-zero.
The post-hoc invariants the trial already checked are untouched — per-run contiguity from 0, one
terminal event per run, one event per `(run_id, seq)` over the settled file.
Three test doubles followed the port: `ClaimTimingStore.claim_resume`, `ClaimStartTimingStore`
and `StallingStore` in the concurrency worker, and `StallingStore` in the crash worker. Each was
overriding a signature that no longer exists, so each would have stopped intercepting; both
`claim_start` overrides now branch on the returned event rather than on `held_by`, which is the
same question asked of the half of the pair that carries the answer.
One measurement fix the port change forced. `ts` comes from SQLite's own clock now (ADR-D11 §4:
N workers on one file must compare one clock, not N), and `strftime('%f')` truncates to whole
milliseconds — measured here, a stamp reads up to 0.999 ms earlier than the instant it was taken.
The two "the claim arrived before the run ended" assertions compare a `time.time_ns()` reading
against that stamp, so they carried a built-in millisecond of bias against themselves and failed
on a race that genuinely happened, about one trial in a hundred. They now compare against the
latest instant the event can have been written at. Observed over four runs of the suite after the
change: 20/20 resume claims overlapping, 10/10 session claims overlapping, 3/10 crossrun trials
genuinely interleaved — the contention is real and reported, not assumed.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ADR-D11 §7's consequence list, landed. Every one of these was a sentence that stayed true only until the port change, and each is now false where it stands: - **coding-standards §6** stated "the Runtime is the **only** assigner of `seq`, one counter per run, recovered from `max(seq)` on resume", carrying an inline note that it stood until the port change and went with it. This is that landing, so the law now reads as the store's. - **`runtime/service.py`'s module docstring** described the per-event order as beginning with "stamp the envelope" and said `seq` and `ts` are stamped there. It says what actually happens and, more usefully, why the refetch promise two lines above it is now true rather than nearly true. - **`_drain`'s docstring** ended "the `seq` that report consumed stays spent, so the log shows a gap … not this arm's to close". Deleted: a refused append takes no number. - **the design doc's envelope-stamping split** gets a dated amendment rather than a rewrite — what it claims about ordering, loss detection and the engine's position is unchanged, and the amendment says which half moved and why the claims become true. The `seq` comment in the `Event` sketch names the store. - **`composition.py`** documents `clock` as reaching nothing that stamps an event, and points at the seam that replaced it: a store built with a clock, or, for the two SQL stores, the backend's own — which is the point, since N workers with N clocks cannot agree on a `stale_after` comparison. `Runtime.__init__` says the same. The index's known-delta rows for all five flip to **Applied 2026-08-08**, and a new row records what this PR deliberately did not do: `clock` is inert on both `Runtime` and `build_runtime`, and removing a public keyword is a breaking change owed its own PR rather than a docstring's. CHANGELOG carries the two user-visible facts: a log no longer keeps a gap after a dropped report or a transient append failure, and `EventStorePort` is a breaking change for anyone who implemented it — eight methods to seven, `append` taking payloads and returning events, both claims returning what they wrote, `last_seq` gone, `SessionClaim.overridden` carrying events, and `stale_before: datetime` becoming `stale_after: timedelta`. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
sagi5060
marked this pull request as ready for review
August 8, 2026 12:16
This was referenced Aug 8, 2026
Review findings on #154. `_CLAIM_ATTEMPTS` sat at 20, exactly `_CONCURRENT_APPENDS`. The WATCH/MULTI/EXEC loop is lock-step — one winner per round dirties every other watcher — so N contenders need exactly N rounds. Measured: N=20 passed 350/350, N=21 failed 60/60. Zero margin, and the failure mode is a permanently red gate, not a flake. 64 now; 60 contenders pass where 21 used to fail. `Runtime(clock=...)` was written to `self._clock` and never read. Silently ignoring a frozen clock is how a caller starts asserting against wall time believing it held time still — the failure this PR had to strip from ~30 call sites. It warns now; removal is its own PR (#158), since dropping a public keyword is breaking. The staleness window is derived from the stamps the store wrote rather than from a fixed 250 ms, so a stall between the two writes widens the window instead of eating it. `_append` on SQLite no longer takes BEGIN IMMEDIATE for an empty payload list, as postgres and redis already did not. Pinned: a run resurrected into `run.interrupted` rather than a terminal event takes its session back and is listed as pending again — the shape ADR-D11 implies but never states. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#157 squash-merged the snapshot sweep to dev with `examples_from` on top; this branch still carried `b65b7d4`'s version of the same lines, so the two conflicted. Dev's is a strict superset — taking it verbatim leaves both sides identical and nothing to resolve. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
sagi5060
added a commit
that referenced
this pull request
Aug 8, 2026
…ntext
Two changes that only make sense together, because the second is what
makes the first reachable by an application at all.
AgentDeck runs agents; it does not model users, organizations or
permissions. Requiring `tenant` and `principal` on every RunContext made
every caller invent values it did not have — a local agent with no tenant
still passed "local", a backend workflow with a workspace but no acting
user still passed "system". Placeholders invented to satisfy an API are
not identity, and carrying them made `tenant` look essential when the
real requirement was only that runs can be kept apart.
`namespace` is that requirement and nothing more: an opaque isolation key
AgentDeck never parses and attaches no meaning to. An application may key
it by workspace, project or business; `None` is a first-class mode, not a
placeholder. `principal` is deleted rather than renamed — nothing ever
read it, and mapping it to Langfuse's user_id rendered a security
principal as the product's user.
A caller no longer builds a context either. RunContext carries the gate,
the reporter and the id the claim protocol addresses runs by; getting one
wrong is a silent no-op, not a type error, since a resume with a fresh
run_id names a run the log has never heard of. So:
await runtime.run("booking-agent", input, session_id="wa:972...", namespace="business:123")
`run_id` is minted unless supplied, and rides on every event, so callers
read it off the stream — `TurnResult` now does exactly that.
Six fields fail one test: does AgentDeck's own machinery read this, or
only write it down? `trace_id` was a second uuid minted beside `run_id`,
never supplied from outside. `budget` was recorded as if it constrained a
run and enforced by nothing, which is worse than absent. `triggered_by`
and `parent_run_id` were read only by telemetry, and nothing originates a
parent — there are no sub-runs. `deadline` and `idempotency_key` had zero
reads anywhere. Each returns with the mechanism that enforces it.
Removing the first three empties RunContextSnapshot, so it and Budget go,
and run.started carries the ask itself.
Envelope v2, declared: removing required fields is a `v` bump, so the
goldens move deliberately and the v1-reader measurement is parked with
its reason — that reader cannot parse a v2 event by construction.
Judgment ledger:
- Stores encode `None` as the empty key via `RunContext.namespace_key`,
defined once in core so four stores cannot disagree; `__post_init__`
refuses an explicit "" so the empty key means one thing.
- SQL stores keep NOT NULL and store the normalized key, because
`WHERE namespace = NULL` never matches.
- `EnginePort` keeps taking a RunContext. Ports are internal and an engine
needs the gate and reporter; only the caller-facing API changed.
- The four fake-identity constant pairs are deleted, retiring the
"local"/"demo" divergence that put one session id in two logs depending
on the route.
- Because #154 moved stamping into the stores, an event cannot carry a
foreign namespace — the cross-namespace write guards are unnecessary
here and were not carried over.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
sagi5060
added a commit
that referenced
this pull request
Aug 11, 2026
… and build_runtime (#208) * fix(runtime,composition): remove the inert clock keyword from Runtime and build_runtime ADR-D11 moved timestamp assignment into the store, so `clock` has decided nothing above the store since #154, which made it accepted-but-inert and added a DeprecationWarning. This deletes the keyword from both signatures (and the warning with it) rather than continuing to carry a trap that reads as "time is held" while deciding nothing. `build_runtime`'s `if clock is None` fork collapses to one `Runtime(...)` call. Holding time still works at the seam that owns it: `MemoryEventStore(clock=...)`, `RedisEventStore(clock=...)`. Breaking change: a caller still passing `clock=` now gets a TypeError instead of a silent no-op, covered by a new regression test. Closes #158. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(changelog): add the migration-notes line for the clock keyword removal The issue asked for a line in the migration notes pointing at the store-side keyword, not just the Changed entry explaining why it's gone. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
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
1 participant
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.
Implements ADR-D11 as amended by #153.
Closes #127.
#121 is no longer closed here. Its snapshot sweep was split out to #157, because #121's own
Notes require it: the sweep "must land as its own PR with the justification stated, never bundled
with a schema change."
b65b7d4is the first commit on this branch, so the split was free — and#157 also carries the "Done when" box this PR left unwritten (a test that regenerates into a tmp
dir with one extra payload and asserts exactly one file moves). Merge #157 first; this branch's
diff then collapses on its own.
Why
seqis both the ordering authority and the loss check, and the second only works if thesequence is dense. The Runtime held the counter and allocation was separated from the write by
an
await, so a refused append burned a number permanently:That contradicted
runtime/service.py's own promise — "a consumer that spots aseqgap canalways refetch it." A seq allocated and persisted in one step cannot be allocated and not
persisted.
test_a_store_that_refuses_a_report_costs_the_report_not_the_runnow assertscheck_contiguous(...) == []on exactly that run; it is the single assertion this PR exists for.What landed
EventStorePort, 8 methods to 7.append(log_key, payloads, ctx, origin) -> list[Event];claim_start(..., origin, stale_after: timedelta) -> (SessionClaim, Event | None);claim_resume(..., resumed: RunResumed, ctx, origin) -> Event | None;last_seqremoved.SessionClaim.overriddencarries each abandoned run's lastEventrather than its id.await_stamp,SQLite's
BEGIN IMMEDIATEon the append path (which it did not take before), Postgres'sadvisory lock, Redis's
WATCH/MULTI/EXECon the per-run ZSET.(spec, ctx, seq)signatures.Shape, per the amended ADR
appendstaysctx-derived. The one write that addresses a foreign run —_close_abandoned— gets what it needs fromSessionClaim.overridden. No override parameter,no new port method, and
claim_startkeeps one job.Clocks: injected callable for memory and redis, backend clock for sqlite and postgres — an
in-process callable gives N workers N disagreeing clocks, which is what
stale_aftercannottolerate.
Judgment ledger
Runtime(clock=...)andbuild_runtime(clock=...)kept but documented as inerttsany more, so both keywords are accepted and forwarded to nothing. Removing a public keyword is a breaking API change and belongs in its own PR, not smuggled into a test conversion. Recorded as an open row in the index's known-deltas so it is not lost. Recommend a follow-up that removes it.clock=lambda: TSstripped from ~30 test call sitestsNOTHING_IS_STALE = timedelta(hours=1)/EVERYTHING_IS_STALE = timedelta(0);timedelta(0)is stale in the right direction on all four backends because equality fails the>comparison. Only the two cases needing one run stale beside a live one arrange a real age gap with a sleep — arranging age, never asserting timing._Held, a hand-moved clock on the store, replaces_abandoned(run_id, ts)TS. Closer to what the store actually sees than a hand-stamped envelope was.tscomes from SQLite's clock now, andstrftime('%f')truncates to whole milliseconds — measured in this repo: a stamp reads up to 0.999 ms earlier than the instant it was taken. The two "the claim arrived before the run ended" assertions therefore carried a built-in millisecond of bias against themselves and failed on a genuine race about one trial in a hundred (it flaked once during this work). They now compare against the latest instant the event can have been written at.LateStore's gate moved fromlast_seqintoclaim_resumeresume()no longer reads the log before claiming, so there was no call left upstream of the claim to gate on. Gating the claim's own entry is strictly stronger — the gate cannot be outrun by a claim it is the first statement of — so theassert self._gate.exists()guard that protected the old ordering is deleted rather than moved._CannotCloserefusesappendforctx.run_id == "r-0"rather thanlast_seqappendwith the abandoned run's context, which is the one thing distinguishing it from this turn's own appends. Same failure, same point in the takeover.Behavior change the ADR implies but never states
A premature takeover no longer kills the run it stepped over. That run used to hold a spent
seq, so its next write was refused and it failed loudly with aStoreError. It holds no numbernow, so its writes are assigned the seqs after the takeover's
run.failedand go through: therun plays to completion and its log ends
[run.started, text.delta, run.failed, run.completed].The damage is still bounded and still detectable, but by
check_terminalreporting two terminalevents rather than by an exception reaching the engine. The log stays dense and no
seqanswersto two events — both structural now instead of enforced. Pinned by
test_a_run_that_writes_again_after_being_taken_over_lands_behind_its_terminal_event. This isthe one thing in the PR a reviewer has to consciously accept rather than merely check.
Tests deleted rather than converted
Each tests something no caller can now express. Named in full in the commit messages.
last_seq— three contract cases, thelast_seqparams in three port-boundaryparametrizations, and one probe inside a tenant-isolation case (which keeps its point through
read_run). The method is off the port.seqrefusals —test_one_seq_per_run_is_refused_a_second_time,test_a_batch_holding_its_own_duplicate_seq_is_refused_whole,test_one_seq_per_run_holds_on_the_claim_paths_too, andconcurrency_worker._refuse_a_spent_seq. No caller supplies aseq, so the duplicate isunconstructible rather than unenforced. What they protected is replaced by ADR-D11 §6's
concurrency case (20 tasks appending to one run at once must come back
[0..19]with noduplicates, on every parametrized backend), and by the crossrun peers now checking per write,
while the peer is still writing, that the number they were handed is their own run's next one.
Measured against a store with one
awaitbetween reading the run's last seq and extending thelog: it hands
0to all twenty, and every other case in the file still passes.test_a_claim_carrying_a_stale_seq_loses_even_though_the_run_is_waiting_again— thestale-
seqhalf ofclaim_resume's contract existed because the caller stamped beforeclaiming. Status is the whole condition now.
test_an_event_stamped_for_another_tenant_is_refused(memory andSQLite) and the
pytest.raises(ValueError, match=<tenant>)halves of two contract cases. Thetenant comes from the
RunContextthat chose the bucket. Every isolation half stays.This supersedes one line of
1e4df66's commit message, which listed the spent-seqrefusalamong the invariants that stay in the crossrun trial. It does not stay; it becomes unreachable.
Doc consequences (ADR-D11 §7)
coding-standards §6,
runtime/service.py's module and_draindocstrings,composition.py'sclockkeyword, and the architecture doc's envelope-stamping split (dated amendment). Theindex's known-delta rows covering all five flip to Applied 2026-08-08; one new row records the inert
clockkeyword. CHANGELOG carries both user-visible facts — no more permanent gaps, and the portchange is breaking for anyone who implemented it.
Golden snapshots were regenerated once, in
b65b7d4, and have not moved since.Gate
make checkgreen in the foreground with both live backends up:All 42 skips are contract-case shape skips (
only a suspended case can be resumed,this run finished) — under CI's ceiling of 44, with Redis and Postgres exercised for real.Not in this PR
#94 (recording the resume value) is a payload change, and coding-standards §7 requires schema
changes in their own PR. It also needs a ruling: store in full, or preview+hash.
🤖 Generated with Claude Code
Review findings, addressed
Reviewed against ADR-D11, #121 and #127. Two blocking, four advisory — all fixed.
_CLAIM_ATTEMPTS(20) equalled_CONCURRENT_APPENDS(20). The WATCH/MULTI/EXEC loop is lock-step, so N contenders need exactly N rounds: measured N=20 → 0 failures in 350 trials, N=21 → 60/60 failures. One more contender turns the case permanently red, not flaky.Closes #121with an unwritten "Done when" box, and 20 snapshot files changed with no stated justification.test_a_run_resurrected_into_an_interrupt_takes_its_session_back.Runtime(clock=...)written toself._clockand never read — a caller freezing it gets no error and silently asserts against wall time.DeprecationWarningwhen passed explicitly; the dead attribute and the now-unused_noware gone. Removal filed as #158, not left as a known-deltas row.AGE_GAPleft a 250 ms machine-dependent window — the class of assertion #127 exists to delete._window_between(older, newer)derivesstale_afterfrom the stamps the store wrote. A stall between the two writes now widens the window instead of eating into it.BEGIN IMMEDIATEeven for an empty payload list, where postgres and redis early-return.The non-terminal resurrection, stated
The paragraph above describes the takeover variant where the stepped-over run completes. The
worse shape is a non-terminal lifecycle event, and it is now pinned:
The run does not merely land behind its own
run.failed— it becomesWAITING_HUMANandreclaims the session a turn was told was free. This is arguably correct (the run really is alive
and really is waiting), which is why the takeover stays advisory rather than fatal, and
check_terminalstill gives it away. It is pinned because it is the shape a reader of ADR-D11would not predict: the ADR says a spent
seqcan no longer refuse a write, and says nothingabout a refused run reclaiming the session it was evicted from.
Verification
make checkin the foreground, then the whole suite again against liveagentdeck-test-redisand
agentdeck-test-pg: ruff clean, ty clean, import-linter 12 kept / 0 broken,979 passed / 42 skipped with both backends live.