fix(manager): restore worker settings after restart - #1225
Conversation
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Removes the Manager startup legacy Worker recreation loop (controller reconciliation owns backend recovery) and adds regression coverage; core change is sound — verified against main: the controller's backend recreation already merges Worker.spec.Env into the backend Create request, and the integration mock exposes FindCreateReq with the recorded Env, so the new assertion is valid. One wiring gap noted below.
Findings
- [Warning] manager/tests/test-manager-startup-recovery.sh:1 — the new regression script is not invoked by any Makefile target or GitHub workflow, so it never runs in CI
Suggestions
Wire manager/tests/test-manager-startup-recovery.sh into an existing test target/workflow so the "controller owns Worker recovery" invariant is actually enforced on future changes.
Automated review by github-manager-bot
|
Addressed in commit 1f640c2. The Manager startup recovery regression is now invoked by the existing Helm Lint workflow, and both the startup script and regression test are included in that workflow's path filters. This keeps the check lightweight and independent of the integration test matrix. Local validation: regression 5/5, bash -n, YAML parse, and git diff --check passed. |
oss-maintainer
left a comment
There was a problem hiding this comment.
LGTM — the follow-up commit (1f640c2) addresses the earlier CI-wiring feedback: manager/tests/test-manager-startup-recovery.sh is now invoked by the Helm Lint workflow, with both the startup script and the regression test included in its pull_request path filters.
Verified against main:
- Controller backend recreation passes
Worker.spec.Envinto the backend Create request (internal/controller/member_reconcile.go), so recovery owned by the controller preserves worker settings; the newTestWorkerPodDeleted_Recreatesassertion (FindCreateReq+ env check) covers this. - Docker local deployment still runs the
agentteams-controllercontainer, so removing the Manager-startup recreation loop leaves recovery coverage in socket mode. - The regression script itself is a sound static guard (grep assertions +
bash -n) for the startup path.
Note for maintainers: the follow-up workflow runs on this fork PR currently sit in action_required and need approval by someone with repo admin rights, which I don't have.
Automated review by github-manager-bot
Closes #575
Summary
Root cause
Manager rebuilt existing Workers from persisted credentials only, which omitted user settings and could race controller reconciliation during restart or upgrade. The controller already has the complete Worker spec and is the correct owner for recovery.
Validation