Skip to content

fix(reporting): settle service lifecycle before resource cleanup - #1218

Merged
bokelley merged 2 commits into
mainfrom
conductor/reporting-service-lifecycle-completion
Sep 26, 2026
Merged

bokelley merged 2 commits into
mainfrom
conductor/reporting-service-lifecycle-completion

Conversation

@bokelley

@bokelley bokelley commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

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_handler receives 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 lint and make typecheck-all passed. The full make test run 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

Comment thread tests/type_checks/reliable_reporting_lifecycle.py Fixed
Comment thread tests/type_checks/reliable_reporting_lifecycle.py Fixed
Comment thread src/adcp/reporting/service_lifecycle.py
Comment thread src/adcp/reporting/service_lifecycle.py
Comment thread tests/test_reliable_reporting_lifecycle.py
Comment thread tests/test_reliable_reporting_lifecycle.py
Comment thread tests/test_reliable_reporting_lifecycle.py
Comment thread tests/test_reliable_reporting_lifecycle.py
Comment thread src/adcp/reporting/service.py Outdated
Comment thread src/adcp/reporting/service.py
aao-secretariat[bot]
aao-secretariat Bot previously approved these changes Sep 25, 2026

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@bokelley
bokelley marked this pull request as draft September 26, 2026 14:54
@bokelley
bokelley force-pushed the conductor/reporting-service-lifecycle-completion branch from 4f70b78 to 84bd392 Compare September 26, 2026 14:54
Comment thread src/adcp/reporting/service_lifecycle.py
Comment thread src/adcp/reporting/service_lifecycle.py
@bokelley
bokelley marked this pull request as ready for review September 26, 2026 15:54

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@bokelley
bokelley merged commit a9719c6 into main Sep 26, 2026
54 checks passed
@bokelley
bokelley deleted the conductor/reporting-service-lifecycle-completion branch September 26, 2026 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant