feat(examples): add durable seller production wiring - #1107
Conversation
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — durable seller production wiring plus one additive, well-tested SDK change.
What I checked:
- The one SDK change (
IdempotencyStore.raise_on_persist_error) is additive; the fail-closed raise correctly releases the advisory lock, TTL/retry-horizon bounds are consistent, and there is no double idempotency application. - New
dedup_keykeys on a requiredidempotency_key. - Largest new files (
durable_tasks.py,test_durable_tasks.py) reviewed; new behavior is test-covered. - No public-surface break, no generated-code hand-edit, no type-layering violation, no CI-gate disabling, no credentials in
ctx_metadata.
Risk flags: high_risk is true only because src/adcp/server/idempotency/store.py was modified and matches src/adcp/server/**. The reviewer found no medium-or-higher concerns on that modified file, so it is presumed safe (no deletions, no gated paths). gated_paths is false, so the REVIEW_REQUIRED status does not block.
Two non-blocking follow-ups noted (worker SIGTERM handling, capability-vs-wrapped-method scope) — neither rises to a finding.
Decision table: no critical/high/medium findings; gated_paths false; no deletions; no author team gate. Falls through to row 9.
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean pass, no blocking or medium findings.
Delta since the prior approve (head 5ae8064) is a single example-code change in examples/v3_reference_seller/src/durable_tasks.py: swapping except BaseException to except Exception in startup/shutdown resource cleanup, plus a first_error annotation. This is the correct shape — it lets CancelledError/KeyboardInterrupt/SystemExit propagate rather than being swallowed, while preserving failure re-raise and best-effort Exception cleanup.
No impact to the public adcp.* surface, wire-shape, forward-compat, import layering, or semver signal. The only high-risk-flagged file (src/adcp/server/idempotency/store.py, modified) drew no medium-or-worse concern from the reviewer, so the modification is presumed contract-preserving.
Decision-table walk: no critical/high findings (row 1 n/a); gated_paths is false (row 2 n/a); no deleted high-risk paths (row 3 n/a); no medium findings at all (rows 4/5/8 n/a); prior decision was approve, not escalate (row 6 n/a); no no-auto-approve team match (row 7 n/a). Falls through to row 9 → approve.
Note: high_risk is true only via a (modified) entry with no findings, and review_decision is REVIEW_REQUIRED but gated_paths is false, so no hard gate applies.
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — subsequent pass on PR #1107, net improvement over a clean prior run.
What I checked:
- Delta adds a reference PostgreSQL WorkflowHandoff queue (
examples/v3_reference_seller/src/workflow_queue.py, +309), signal-driven worker shutdown, a restart-recovery conformance test, docs, and a message-only SDK change insrc/adcp/server/idempotency/store.py. Reviewer confirms lease/claim/ack semantics are correct and the store.py change is behavior-neutral (no public-surface signature/shape change → no semver signal required). - High-risk flag is true only because
store.pyis(modified)undersrc/adcp/server/**. The single reviewer Medium (unbounded retry of non-transient failures with no attempt cap / dead-letter) lands on the reference example queue, not onstore.py, so Row 5 (high-risk modified + medium finding) does not fire — the modified sensitive file carries no medium-or-higher concern.
Decision trace:
- Row 1 (critical/high): none.
- Row 2:
gated_pathsfalse — N/A. - Row 3/4/5: no deletions; the lone medium is not data-loss/schema/infra and not on the modified high-risk file → no escalate trigger.
- Row 6: prior decision was
approve, notescalate— sticky N/A. - Row 7: no no-auto-approve team match.
- Row 8: only 1 medium finding (<3).
- Row 9: approve.
No no-auto-approve team gate; review_decision REVIEW_REQUIRED does not block approve when gated_paths is false. One medium (reference-example retry cap) is non-blocking.
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — subsequent pass over the retry-bounding delta (commit 90ff4bc).
Checked:
- No public
adcp.*surface, generated types, or wire-shape changes in this diff; changes are confined to the example v3_reference_seller workflow queue/worker plus tests/docs and the idempotency store. - Retry/attempt boundary, capped exponential backoff, cross-account fail-closed dead-lettering, crash-recovery reconciliation, concurrent DDL locking, and startup cancellation all traced and covered by conformance + unit suites.
- high_risk is true only because
src/adcp/server/idempotency/store.py(modified) matchessrc/adcp/server/**; no medium-or-higher finding on that file, so the modification is presumed safe (no row-5 trigger). - gated_paths is false; no no-auto-approve team match.
Decision table: no critical/high findings (row 1 n/a), gated_paths false (row 2 n/a), no deletions (row 3 n/a), no medium findings (rows 4/5/8 n/a), prior decision was approve (row 6 n/a), no team gate (row 7 n/a) → falls through to row 9: approve. Consistent with the prior approve. One Low (state-constraint name truncation for table names >51 chars) left unposted per severity policy.
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean pass, no blocking or medium findings.
What I checked
- No critical/high/medium findings from the reviewer. The delta is confined to
examples/v3_reference_seller(worker.py plus two tests), outside the publicadcp.*surface. - The
worker.pyfix keys shutdown detection offcompleted_workers = done.intersection(workers)and re-raisestask.exception()before theRuntimeErrorfallback, so a worker crash surfaces even when a stop signal lands in the sameFIRST_COMPLETEDcycle — fail-closed, with thefinallystill cancelling and gathering all tasks. New testtest_worker_failure_wins_when_shutdown_is_also_readypins the race. test_durable_tasks.pychange is a behaviorally equivalent test-only rewrite.
High-risk flag: src/adcp/server/idempotency/store.py is flagged (modified, matches src/adcp/server/**), but the reviewer surfaced no medium-or-higher concern on it, so the modification is presumed safe. Row 5 does not fire without a medium finding.
Gates: gated_paths false; no no-auto-approve team match; prior decision was approve. No decision-table row 1–8 fires → row 9 approve.
Summary
WorkflowHandoffqueue with account scoping, restart recovery, capped exponential backoff, bounded attempts, and dead-letteringcreate_schema()bootstrapping from production-owned versioned migrations, including the reference queue's additive dead-letter schema upgradeVerification
make lint typecheckExpert review
Independent code, protocol, and security reviews completed with no remaining blockers. Review feedback stayed local; no GitHub review comments were posted.