Repository navigation
fix(deploy): report deploy state with drift, and refuse destructive resolve mid-play (#13913) - #13946
Conversation
…esolve mid-play (#13913) A drift reading taken while ansible is mid-write reports files that are simply being copied. Measured across one self-update: 28 of 30 reported drifts evaporated once the play settled — same host, same endpoint, ten minutes apart, nothing resolved in between. Both checksums were populated throughout, so the source_checksum: null heuristic from #13851 does not filter them out; they look exactly like genuine content drift. The window where the answer is wrong is the window where operators look, and the remediation is a delete-style rsync, so acting on a mid-deploy reading is destructive against files that are mid-write. The signal is the transient systemd service the detached run lives in (#11492/#12596). It is forked by PID 1 rather than by this backend, so unlike any in-process flag it survives the 'systemctl restart autobot-slm-backend' that Play 1 performs halfway through the run being tracked. The self-update log cannot answer this: complete=False means both 'still running' and 'died', which are opposite answers for a caller deciding whether to trust a reading — the same reason /status reported self_update_incomplete: false throughout. Unknown is a third state. A query that could not run reports None, never False: 'nobody could check' and 'checked, nothing running' are different claims and only the second makes a reading trustworthy. Unknown does NOT block resolve. On a host where the unit cannot be queried, refusing would permanently disable remediation — a regression against working behaviour — whereas proceeding is exactly what happened before this guard. It is logged and surfaced in the response instead. The probe is also wrapped: a safety check must never be the thing that 500s the operation it protects. Verification: 9 mutations killed (unknown->False in each of the four failure paths, an incomplete run dated as a completion, unstable-when-unknown, a hardcoded unit prefix, the guard removed from the async resolve path, a required schema field). The unit-prefix guard is asserted through the AST because the first, substring-based version let the mutation through. 843 existing tests in tests/api and tests/services pass unchanged.
Applied Black, isort, and autoflake to match code-quality checks. Triggered by workflow auto-fix-formatting.yml.
✅ SSOT Configuration Compliance: Passing🎉 No hardcoded values detected that have SSOT config equivalents! |
…ds (#13913) verify-generated-types was red: adding deploy_in_progress, deploy_state_reason and last_completed_play_at to FileDriftReport changes the OpenAPI schema, and the checked-in TypeScript was still describing the old shape. Regenerated rather than hand-edited, and the diff is confined to the three fields — additive only, no unrelated drift picked up from the base: openapi.json 33 insertions, 1 deletion api.ts 6 insertions, 0 deletions
|
Two failures, two different causes.
Fixing them inline would mean doing #13149 inside an unrelated PR, against the paths that decide where deploys write. I checked for a baseline or exemption mechanism first — there is none; the hook's allowlist is path-pattern only. This is the fifth PR hit by the file-vs-line scoping in one session, and it is the one where the inherited backlog is too large and too dangerous to absorb. #13962 scopes the guard to changed lines (opt-in, fails closed, 3 mutations killed) and unblocks this and #13945. |
|
Follow-up filed from this PR: #14006 — That is why the response-shape tests here capture the kwargs passed to |
1 participant
Closes #13913
Thinking Path
The endpoint was not wrong about the checksums — during a play the two trees genuinely differ. It was wrong about what that means, and it had no way to say so. So the fix is not a smarter comparison but an honest annotation plus a guard on the one path that does damage.
Three candidate signals, two rejected:
complete=Falsemeans both still running and died — opposite answers for a caller deciding whether to trust a reading. Same reason/statusreportedself_update_incomplete: falsethroughout the measured window: that verdict describes the previous runWhat Changed
services/deploy_activity.py(new) —read_deploy_activity()returnsin_progress, areason, andlast_completed_play_at.Unknown is a third state. A query that could not run reports
None, neverFalse. "Nobody could check" and "checked, nothing running" are different claims, and only the second makes a drift reading trustworthy — collapsing them would restore the exact silence this issue is about.GET /driftcarriesdeploy_in_progress/deploy_state_reason/last_completed_play_at. All optional withNonedefaults, so no existing caller breaks. The activity is read after the comparison: a play that starts mid-scan still makes the reading unstable, and checking afterwards catches that ordering.POST /drift/resolveand/resolve-async409 while a play is running. One shared helper, not two copies — a test asserts exactly two call sites.Two decisions worth reviewing
Unknown does not block resolve. On a host where the unit cannot be queried, refusing would permanently disable remediation — a regression against behaviour that works today — whereas proceeding is exactly the pre-existing behaviour. Logged, and surfaced in the drift response.
The probe is wrapped in try/except.
read_deploy_activityis written never to raise, so this is defence in depth; it earns its four lines because a probe added to make a destructive operation safer must never be the thing that takes it down.Verification
9 mutations applied, 9 killed — the fix reverted piece by piece:
None→Falseon systemctl-missingtest_a_query_that_could_not_run_is_unknown_not_falseNone→Falseon timeouttest_an_unfinished_run_is_not_dated_as_a_completionreadings_are_unstabletreats unknown as unstabletest_the_unit_pattern_is_derived_from_the_executor_not_restatedtest_both_resolve_paths_share_one_guardtest_resolve_refuses_while_a_play_is_runningFalseat the endpointtest_an_unknown_deploy_state_reaches_the_caller_as_nonetest_the_schema_declares_the_three_fields_as_optionalTwo things the first drafts got wrong, worth recording because both were vacuous passes:
from services import deploy_activityreturns a conftest MagicMock, under which_last_completed_play_at(...) is Nonepassed while asserting nothing. Real-loaded by path instead."autobot-selfupdate-*.service"sailed past it. It now reads the assignment's AST and asks whether the value is built from the imported name.Not in this PR
The issue's "separate finding" —
autobot-slm-agentreporting a permanent 2-of-8 false-positive rate because the role deliberately never ships its*_test.pyfiles — is adeployed_checksum: nullclass, the mirror of #13851's two. Fixing it properly means deriving each component's shipped set from the role'scopy:tasks rather than assuming the whole source tree deploys, which is a different module and a different risk. Spun out rather than bolted on.Model Used
claude-opus-5[1m]