fix(reporting): settle service lifecycle before resource cleanup - #1218
Conversation
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — reporting service lifecycle refactor (settlement barriers, owned-resource lifecycle, fail-stop supervision).
What I checked:
- Settlement/drain machinery with cancellation/PG/MCP tests verified sound by the reviewer.
- Two Medium contract/blast-radius observations were posted inline (config unexpected-exception fail-stop across tenants; changed public worker-error contract under
fix:prefix), but no findings landed in the findings list.
Decision path: No critical/high findings (row 1 no). gated_paths is false (row 2 no). high_risk false with no deletions (rows 3, 5 no). No medium findings with data-loss/schema/infra category in the findings list (row 4 no). No prior escalation (row 6 no). No no-auto-approve team match (row 7 no). Fewer than three medium findings recorded in the findings list (row 8 no). Falls through to row 9 → approve.
Note: the two inline Mediums touch a public worker-error contract change shipped under a fix: prefix; a human may want to confirm the semver signal is appropriate, but this was surfaced as an inline observation, not a recorded finding, and does not meet any blocking or escalation row.
4f70b78 to
84bd392
Compare
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — reporting-lifecycle rework is clean and well tested.
Checked: new _settlement.py and service_lifecycle.py, the service.py restructure, and the producer.py/inline_source.py cancellation changes. The reviewer confirms the settlement barrier, drain ordering, admission gating, and log/error redaction are correct with test coverage. No Critical/High/Medium findings.
Gate checks: gated_paths is false; high_risk is false; no no-auto-approve team match; review_decision is REVIEW_REQUIRED but no hard gate applies (row 2 only fires when gated_paths is true). Prior decision was approve — this is a net-neutral/positive follow-up. Zero blocking or medium findings means rows 1–8 do not fire; falls through to row 9.
One non-blocking Low note (omitted from gate): the additive public API (new exports, close(timeout=)/wait()/state/ready/failure, owned_resources) ships under a fix: prefix, which release-please cuts as a patch rather than a minor. This is a semver-signal accuracy nit, not a breaking-change violation — nothing is removed, renamed, or retyped — so it does not meet the High bar for the mandatory semver rule. Worth tightening the commit prefix to feat: before merge.
Reporting shutdown could close resources while an admitted RPC, worker turn, or cancelled database/source operation was still settling. The service now closes admission at STOPPING, drains owned work, and then closes explicitly owned resources once in reverse order. Cancelled waiters and shutdown deadlines leave the shared drain running until cleanup finishes.
Configuration and extension failures retain the existing isolation policy: other work continues, the next turn retries, and readiness stays available. Returned turns preserve their error maps, and
worker_error_handlerreceives the detailed component name and original exception. Startup, scheduler, and resource cleanup failures remain terminal;wait()reports a sanitized lifecycle error, and SDK logs omit account identifiers and exception bodies.Validation:
make lintandmake typecheck-allpassed. The fullmake testrun passed with 11,643 tests, 2,125 skips, 9 deselections, 1 expected failure, and 81.70% coverage. An additional focused run passed 175 tests, including real PostgreSQL error recovery in both transaction modes and cancellation/cleanup ordering. Intentional awaits and cancellation boundaries flagged by CodeQL are explained in their resolved review threads.Refs #1172.
Open workspace in Conductor