Skip to content

Commit 17799c1

Browse files
committed
fix: address PR #89 review — journal identity isolation, 429 handling, flush deadline
Addresses all four review findings: 1. [P1] Replay isolated by credentials: journal records now live in a per-identity subdirectory keyed by a non-secret truncated SHA-256 fingerprint of (base_url, api_key). Co-located clients with different servers or API keys can no longer replay each other's records — a foreign record previously got 401/403 and was discarded as terminal, permanently losing the spend. 2. [P1] HTTP 429 / LIMIT_EXCEEDED is transient, not terminal: both the commit and event classifiers now detect rate limiting (status 429 or error code), retain the journal entry, keep retrying, and make the next attempt wait at least the server's Retry-After (consumed once, max'd against the normal backoff). Consistent with ErrorCode.is_retryable, which already classified LIMIT_EXCEEDED retryable. 3. [P2] Replay claim scoped to the identity subdirectory: because the claim now covers exactly one (server, credential) identity, an engine for server A can never block server B's entries from replaying out of a shared journal_dir. 4. [P2] Process-wide flush deadline: _flush_all_engines() computes one deadline from the max engine timeout and passes each engine only the remaining budget, so worst-case shutdown is retry_flush_timeout, not engine_count x retry_flush_timeout. No journal-layout migration needed — v0.5.0 is unreleased. 469 tests pass at 100% coverage; ruff and mypy --strict clean. AUDIT.md, CHANGELOG.md, README.md updated.
1 parent 572241c commit 17799c1

6 files changed

Lines changed: 271 additions & 28 deletions

File tree

AUDIT.md

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
# Cycles Protocol v0.1.25 — Client (Python) Audit
22

3-
**Date:** 2026-07-27 (v0.5.0 — durable commit retries: on-disk pending-commit journal with next-run replay, bounded atexit flush, and `POST /v1/events` recovery for commits that land after reservation expiry; async retry-task GC fix; `retry_enabled=False` now journals instead of silently dropping. See the dated entry below. 460 tests pass at 100% coverage.),
3+
**Date:** 2026-07-27 (v0.5.0 — durable commit retries: on-disk pending-commit journal with next-run replay, bounded atexit flush, and `POST /v1/events` recovery for commits that land after reservation expiry; async retry-task GC fix; `retry_enabled=False` now journals instead of silently dropping. Review hardening: per-identity journal partitioning, 429 transient with `Retry-After`, process-wide flush deadline. See the dated entry below. 469 tests pass at 100% coverage.),
44
2026-07-10 (v0.5.0 — `TENANT_CLOSED` + `LIMIT_EXCEEDED` error-code support. `TENANT_CLOSED` per runtime spec v0.1.25.13 (`cycles-protocol-v0.yaml`, runcycles/cycles-protocol#125): `ErrorCode.TENANT_CLOSED` enum member, `TenantClosedError` subclass wired into the lifecycle error-code→exception mapping (reservation-creation surfaces), `CyclesProtocolError.is_tenant_closed()` helper. `LIMIT_EXCEEDED` per runtime spec v0.1.25.12 (revision 2026-07-04, HTTP 429 rate limiting): enum-only member matching the `BUDGET_FROZEN`/`BUDGET_CLOSED` pattern, classified retryable at both the enum and exception layers (429 is transient; previously it fell through to `UNKNOWN`, which happened to be retryable, so semantics are unchanged — now typed). Enum reordered to mirror spec declaration order. Both purely additive; previously both codes fell through the `ErrorCode.from_string` forward-compat path to `UNKNOWN`. See the dated entries at the end of this file. 398 tests pass at 100% coverage.),
55
2026-07-09 (README + docstring transport-error documentation fix, no version bump — see the dated entry at the end of this file. `CyclesTransportError` is exported but never raised by the SDK; README and its docstring now describe the actual `status == -1` surfacing.),
66
2026-07-03 (integration-test-only, no version bump — `test_health_check` now probes the public `/actuator/health/readiness` endpoint instead of aggregate `/actuator/health`, which requires `X-Admin-API-Key` since cycles-server v0.1.25.45 and fails closed with 500 when the server has no admin key configured. The old assertion had failed the org nightly Full-Stack Integration every night since 2026-06-28. No library code change.),
@@ -23,7 +23,14 @@ has already returned the reserved budget to the pool — is recovered via
2323
`POST /v1/events` (spec-conformant `EventCreateRequest`, commit idempotency
2424
key reused, recovery markers in `metadata`). Also fixes the async engine's
2525
unreferenced-task GC hazard and the silent drop under `retry_enabled=False`.
26-
460 tests pass at 100% coverage.
26+
Post-review (PR #89) hardening: journal records are partitioned into
27+
per-identity subdirectories (SHA-256 fingerprint of base_url + api_key) so
28+
co-located clients with different credentials never replay or 401-discard
29+
each other's records and replay claims cannot cross identities; HTTP 429 /
30+
`LIMIT_EXCEEDED` is transient (record retained, `Retry-After` honored)
31+
instead of a terminal discard; the atexit flush enforces one process-wide
32+
`retry_flush_timeout` deadline instead of per-engine. 469 tests pass at
33+
100% coverage.
2734

2835
## 2026-07-26 — Python publishing workflow maintenance
2936

CHANGELOG.md

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,9 +11,10 @@ Durable commit retries. Previously a commit that failed transiently lived only i
1111

1212
### Added
1313

14-
- `runcycles.journal`: file-per-commit `CommitJournal` (atomic write, idempotent replay). Config: `journal_enabled` (default `True`), `journal_dir` (default `~/.runcycles/commit-journal`), `retry_flush_timeout` (default 10 s); env `CYCLES_JOURNAL_ENABLED`, `CYCLES_JOURNAL_DIR`, `CYCLES_RETRY_FLUSH_TIMEOUT`. The first engine created per journal directory replays surviving entries for its `base_url`; corrupt files are renamed `*.corrupt` for operator triage.
14+
- `runcycles.journal`: file-per-commit `CommitJournal` (atomic write, idempotent replay). Config: `journal_enabled` (default `True`), `journal_dir` (default `~/.runcycles/commit-journal`), `retry_flush_timeout` (default 10 s); env `CYCLES_JOURNAL_ENABLED`, `CYCLES_JOURNAL_DIR`, `CYCLES_RETRY_FLUSH_TIMEOUT`. Records are partitioned into per-identity subdirectories keyed by a non-secret SHA-256 fingerprint of `(base_url, api_key)`, so clients with different servers or credentials sharing a journal directory never replay — or 401-discard — each other's records, and one identity's replay claim cannot starve another's. The first engine created per identity replays surviving entries; corrupt files are renamed `*.corrupt` for operator triage.
1515
- Event fallback: when a commit (first attempt or retry) returns `RESERVATION_EXPIRED`, the SDK posts the spend to `/v1/events` reusing the commit's idempotency key, with `metadata.recovered_reservation_id` / `metadata.recovery_reason` markers and no `overage_policy` (spec default `ALLOW_IF_AVAILABLE` never rejects). Applies to the `@cycles` lifecycles and both streaming context managers. `RESERVATION_FINALIZED` is still treated as settled.
16-
- `flush()` on both retry engines; a process-wide `atexit` hook flushes sync engines for up to `retry_flush_timeout` seconds so daemon retry threads aren't killed mid-backoff on clean exit.
16+
- `flush()` on both retry engines; a process-wide `atexit` hook flushes sync engines under one shared `retry_flush_timeout` deadline (not per engine) so daemon retry threads aren't killed mid-backoff on clean exit and shutdown time stays bounded regardless of engine count.
17+
- Rate-limit awareness in the retry engines: HTTP 429 / `LIMIT_EXCEEDED` on a commit or event attempt is transient — the journal entry is retained and the next attempt waits at least the server's `Retry-After` (consistent with `ErrorCode.is_retryable`).
1718

1819
### Fixed
1920

README.md

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -237,12 +237,19 @@ CyclesConfig(
237237
238238
A commit records spend that has already happened, so the SDK never lets one
239239
exist only in memory. Every commit scheduled for background retry is first
240-
journaled to disk (`journal_dir`, default `~/.runcycles/commit-journal`) and
241-
removed only on a terminal outcome:
240+
journaled to disk and removed only on a terminal outcome. Records live under
241+
`journal_dir` (default `~/.runcycles/commit-journal`) in a per-identity
242+
subdirectory keyed by a non-secret fingerprint of `(base_url, api_key)`, so
243+
clients using different servers or credentials on the same machine never
244+
replay — or discard — each other's records:
242245
243246
- **Process exit**: an `atexit` hook waits up to `retry_flush_timeout` seconds
244-
for in-flight retries; anything unfinished stays journaled and is replayed
245-
automatically the next time the process creates a client lifecycle.
247+
(one process-wide budget shared across all engines) for in-flight retries;
248+
anything unfinished stays journaled and is replayed automatically the next
249+
time the process creates a client lifecycle.
250+
- **Rate limiting**: HTTP 429 / `LIMIT_EXCEEDED` responses are transient —
251+
the journal entry is kept and the next attempt waits at least the server's
252+
`Retry-After`.
246253
- **Reservation expired before the commit landed**: the server has already
247254
returned the reserved budget to the pool, so the SDK re-records the spend
248255
via `POST /v1/events` (the protocol's post-hoc direct-debit endpoint),

runcycles/journal.py

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515

1616
from __future__ import annotations
1717

18+
import hashlib
1819
import json
1920
import logging
2021
import time
@@ -33,6 +34,19 @@ def default_journal_dir() -> Path:
3334
return Path.home() / ".runcycles" / "commit-journal"
3435

3536

37+
def auth_fingerprint(base_url: str, api_key: str) -> str:
38+
"""Non-secret identity for one (server, credential) pair.
39+
40+
Journal records are stored under a per-identity subdirectory so that
41+
clients sharing a journal directory but using different servers or API
42+
keys never replay (and on 401/403, discard) each other's records. A
43+
truncated SHA-256 is not reversible and API keys are high-entropy, so
44+
the fingerprint is safe to use as a directory name.
45+
"""
46+
digest = hashlib.sha256(f"{base_url}\n{api_key}".encode()).hexdigest()
47+
return digest[:16]
48+
49+
3650
def _safe_filename(reservation_id: str) -> str:
3751
sanitized = "".join(c if c.isalnum() or c in "-_" else "_" for c in reservation_id)
3852
return f"{sanitized}{_SUFFIX}"

runcycles/retry.py

Lines changed: 54 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,9 @@ class _PendingCommit:
3737
event_fallback_body: dict[str, Any] | None = None
3838
mode: str = "commit" # "commit" | "event"
3939
attempt: int = 0
40+
# Server-requested minimum delay (seconds) before the next attempt,
41+
# set from a 429's Retry-After and consumed by the retry loop.
42+
retry_after_s: float | None = None
4043

4144

4245
def _extract_error_code(response: CyclesResponse) -> str | None:
@@ -47,9 +50,12 @@ def _extract_error_code(response: CyclesResponse) -> str | None:
4750
return raw if isinstance(raw, str) else None
4851

4952

50-
# Journal replay must happen at most once per journal directory per process:
51-
# the first engine created for a directory claims it and replays surviving
52-
# entries; later engines (and the claimer's own in-flight work) are excluded.
53+
# Journal replay must happen at most once per identity directory per process:
54+
# the first engine created for a (server, credential) identity claims its
55+
# subdirectory and replays surviving entries; later engines (and the
56+
# claimer's own in-flight work) are excluded. Because the claim is scoped to
57+
# the identity subdirectory — not the shared parent — an engine for one
58+
# server/key can never block another identity's entries from replaying.
5359
_replay_lock = threading.Lock()
5460
_replayed_dirs: set[Path] = set()
5561

@@ -72,8 +78,18 @@ def _claim_replay(directory: Path) -> bool:
7278

7379

7480
def _flush_all_engines() -> None:
75-
for engine in list(_live_engines):
76-
engine.flush()
81+
# One process-wide deadline: retry_flush_timeout bounds the whole exit
82+
# wait, not each engine. With several engines (decorators, streams) the
83+
# remaining budget shrinks as each is flushed.
84+
engines = list(_live_engines)
85+
if not engines:
86+
return
87+
deadline = time.monotonic() + max(engine._flush_timeout for engine in engines)
88+
for engine in engines:
89+
remaining = deadline - time.monotonic()
90+
if remaining <= 0:
91+
return
92+
engine.flush(remaining)
7793

7894

7995
def _register_engine_for_flush(engine: CommitRetryEngine) -> None:
@@ -99,8 +115,12 @@ def __init__(self, config: CyclesConfig) -> None:
99115
self._client: Any = None # set by lifecycle to avoid circular import
100116
self._journal: CommitJournal | None = None
101117
if config.journal_enabled:
102-
directory = Path(config.journal_dir) if config.journal_dir else _journal.default_journal_dir()
103-
self._journal = CommitJournal(directory)
118+
base = Path(config.journal_dir) if config.journal_dir else _journal.default_journal_dir()
119+
# Per-identity subdirectory: clients sharing a journal directory
120+
# but using different servers or API keys must never replay each
121+
# other's records (a foreign record would 401/403 and be
122+
# discarded as terminal — permanent spend loss).
123+
self._journal = CommitJournal(base / _journal.auth_fingerprint(config.base_url, config.api_key))
104124

105125
def _journal_record(self, pending: _PendingCommit) -> None:
106126
if self._journal is not None:
@@ -151,8 +171,27 @@ def _log_disabled_drop(self, pending: _PendingCommit) -> None:
151171
pending.mode, pending.reservation_id,
152172
)
153173

154-
def _delay_for(self, attempt: int) -> float:
155-
return min(self._initial_delay * (self._multiplier**attempt), self._max_delay)
174+
def _delay_for(self, pending: _PendingCommit) -> float:
175+
delay = min(self._initial_delay * (self._multiplier ** pending.attempt), self._max_delay)
176+
if pending.retry_after_s is not None:
177+
# A 429 asked us to wait: honor the server's Retry-After when it
178+
# exceeds our own backoff, then clear it — it applies once.
179+
delay = max(delay, pending.retry_after_s)
180+
pending.retry_after_s = None
181+
return delay
182+
183+
def _is_rate_limited(self, pending: _PendingCommit, response: CyclesResponse) -> bool:
184+
"""Detect 429 / LIMIT_EXCEEDED and stash its Retry-After. Transient, never terminal."""
185+
if response.status != 429 and _extract_error_code(response) != "LIMIT_EXCEEDED":
186+
return False
187+
retry_after_ms = response.retry_after_ms_header
188+
if retry_after_ms is not None:
189+
pending.retry_after_s = retry_after_ms / 1000.0
190+
logger.warning(
191+
"%s retry rate-limited: reservation_id=%s, attempt=%d, retry_after_ms=%s",
192+
pending.mode, pending.reservation_id, pending.attempt, retry_after_ms,
193+
)
194+
return True
156195

157196
def _classify_commit_response(self, pending: _PendingCommit, response: CyclesResponse) -> bool:
158197
"""Handle a commit attempt's response. Returns True when terminal.
@@ -167,6 +206,8 @@ def _classify_commit_response(self, pending: _PendingCommit, response: CyclesRes
167206
)
168207
self._journal_discard(pending.reservation_id)
169208
return True
209+
if self._is_rate_limited(pending, response):
210+
return False
170211
if response.is_client_error:
171212
code = _extract_error_code(response)
172213
if code == "RESERVATION_EXPIRED":
@@ -207,6 +248,8 @@ def _classify_event_response(self, pending: _PendingCommit, response: CyclesResp
207248
)
208249
self._journal_discard(pending.reservation_id)
209250
return True
251+
if self._is_rate_limited(pending, response):
252+
return False
210253
if response.is_client_error:
211254
logger.error(
212255
"Event fallback rejected (%s); spend recovery failed: reservation_id=%s, status=%d",
@@ -306,7 +349,7 @@ def flush(self, timeout: float | None = None) -> None:
306349

307350
def _retry_loop(self, pending: _PendingCommit) -> None:
308351
while pending.attempt < self._max_attempts:
309-
delay = self._delay_for(pending.attempt)
352+
delay = self._delay_for(pending)
310353
pending.attempt += 1
311354
logger.info(
312355
"Scheduling %s retry: reservation_id=%s, attempt=%d/%d, delay=%.1fs",
@@ -422,7 +465,7 @@ async def flush(self, timeout: float | None = None) -> None:
422465

423466
async def _retry_loop(self, pending: _PendingCommit) -> None:
424467
while pending.attempt < self._max_attempts:
425-
delay = self._delay_for(pending.attempt)
468+
delay = self._delay_for(pending)
426469
pending.attempt += 1
427470
logger.info(
428471
"Scheduling async %s retry: reservation_id=%s, attempt=%d/%d, delay=%.1fs",

0 commit comments

Comments
 (0)