Skip to content

fix(detection-engine): only back up state.db when this run will install the binary - #190

Merged
kjelly merged 11 commits into
mainfrom
fix/detection-engine-always-backup
Oct 31, 2026
Merged

fix(detection-engine): only back up state.db when this run will install the binary#190
kjelly merged 11 commits into
mainfrom
fix/detection-engine-always-backup

Conversation

@kjelly

@kjelly kjelly commented Oct 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Owner

Refs #189

問題

pilot deploy --limit 的 apply，最後的冪等性檢查會因為 it-core 上 detection-engine 的 Step 4: back up state.db before an upgrade 在第二次套用又變更而失敗（idempotency failed: 第二次套用仍有 1 個變更），即使所有主機 failed=0。實際 run：infra-config 的 Deploy，prod apply，--limit lkvs-b200-u08,p6k-digitree,LKVS-B200-U05（run 40850585606，log 第 24893 行是那一輪唯一的 changed:）。

原因

pilot 對 --limit 的 run 實際執行 ansible-playbook site.yml --limit <三台>,freeipa,it-core --tags audit-log-forwarding,dcgm-exporter,docker,freeipa-client,...。it-core 是依賴展開帶進來的提供者主機，detection-engine 不在 --tags 裡，所以真正換 binary 的 Step 6（標 detection-engine-C1，並帶 site.yml 給的 observability/detection-engine）不會跑。但 Step 3–5 標 always（為了讓任何跑到 Step 6 的 tag-scoped run 都先有備份），仍然執行：

  • it-core 的 binary 比 image 內的 artifact 舊（沒有 --limit 的完整 preview 會列出 Step 6 要更換它），detection_engine_is_upgrade 為真；
  • Step 6 不跑，binary 永遠是舊的，is_upgrade 永遠為真；
  • Step 4 是 changed_when: true，每一次套用都有一個變更。

做法

Step 3、3b、3c、3d、4、5 的 tag 從 always 改成 Step 6 自己的 detection-engine-C1（observability、detection-engine 由 site.yml 的 import 繼承，與 Step 6 相同）。這樣 Ansible 在所有選擇下（沒有 tags、--tags tagged、--skip-tags、component tag、detection-engine-C1…）都是「選到 Step 6 就選到備份，沒選到就都不跑」。Step 3c 的判斷式維持原樣。

rescue（維持 tags: [always]）原本用 detection_engine_is_upgrade | default(false) 讀這個變數，但 setter 不再是 always，TestRegression_AlwaysTaggedTasksHaveAllPrerequisitesAlways 會（正確地）拒絕這種寫法；也不放寬或加 allowlist。改成：

  • 「restore the previous binary」與「restore the pre-upgrade database」：when 同時要求 detection_engine_binary_result is defined（Step 6 的 register，Step 6 失敗也會註冊，沒選到或沒跑到就未定義，即「這個 run 到過 Step 6，binary 可能被換掉、備份是這個 run 自己拍的」）與 detection_engine_is_upgrade is defined and detection_engine_is_upgrade；
  • 「restart with the restored previous binary」那個 task 不再讀 detection_engine_is_upgrade，改成依下面的 Step 0 快照還原 service 與設定檔（見「service 狀態的還原」）。

binary_result is defined 同時修掉一個原本就有的問題：Step 4 失敗（Step 6 沒跑到）時，rescue 會把更早一次升級留下的 .prev／pre-upgrade.db 蓋回現在的 binary 與 state.db。

（第一版用自己寫的 ansible_run_tags 判斷式，沒涵蓋 tagged 這個特殊選擇器和 --skip-tags，被審查指出 --tags tagged 會升級 binary 卻沒備份、失敗也無法復原；改成直接沿用 Step 6 的 tag，不再自己重現 Ansible 的選擇規則。）

驗證

  • ansible-playbook --syntax-check 通過。
  • 新增 TestDetectionEngineBackupFollowsStep6TagSelection：用真的 ansible-playbook 跑這個 play 的 Step 3–6（真實 task、只把 root 擁有者拿掉並用假的 binary），對 9 種選擇各跑一次，檢查「Step 6 換了 binary ⇔ 兩份備份（pre-upgrade.db、舊 binary）都存在」：沒有 tags、--tags tagged、--tags detection-engine-C1、--tags detection-engine、--tags observability 會升級並備份；--skip-tags tagged、--tags detection-engine --skip-tags detection-engine-C1、只選其他元件（失敗 run 的情況）、detection-engine-C2 不升級也不備份。
  • 同一個測試對上一版（判斷式版本）會在 --tags tagged（升級卻沒備份）和 --tags detection-engine --skip-tags detection-engine-C1（沒升級卻備份）失敗，對這一版通過。
  • 新增 cmd/pilot/cmd/detection_engine_rollback_tags_test.go：
    • TestDetectionEngineBackupTagsEqualStep6Tags：不需要 ansible 的 YAML 靜態檢查，Step 3、3b、3c、3d、4、5 的 tag 集合必須等於 Step 6 的（且不含 always）；三個 rescue 還原/重啟 task 要是 always 並帶 is defined 守衛（兩個還原 task 要有 detection_engine_binary_result is defined）。
    • TestDetectionEngineRollbackFollowsStep6：真 ansible-playbook 跑真實 Step 3–6 與兩個真實還原 task，中間注入改 state.db 的 task 與 Step 6 之後的失敗。沒有 tags、--tags tagged、--tags detection-engine-C1、--tags detection-engine、--tags observability（含預先放了舊備份的情況）都要還原成舊 binary 與原本的 db（用的是這個 run 拍的備份）；沒選到 Step 6 的 run（--tags detection-engine --skip-tags detection-engine-C1 加失敗、--tags detection-engine-C4 加失敗，皆預放舊備份）與 Step 4 失敗（舊 binary 的 db backup 退出 1，預放舊備份）什麼都不還原。
    • TestDetectionEngineSecondApplyChangedZero：舊 binary，沒有 tags 與 tagged：第一次 apply 變更 Step 4、5、6（changed=3），第二次 changed=0；--tags docker 對舊 binary：沒有任何 detection-engine task 執行、沒有備份、binary 不變。
  • 負向對照：對 base（d013ed95）的 playbook，--tags detection-engine --skip-tags detection-engine-C1／只選其他元件的備份測試、舊備份的 rollback 測試與 --tags docker 第二次 apply 測試失敗；對上一個 head（2aad47cd）的 playbook，Step 4 失敗且有舊備份的案例與靜態守衛失敗；對這一版通過。
  • TestRegression_AlwaysTaggedTasksHaveAllPrerequisitesAlways、TestRegression_RescueRunsWheneverItsBlockDoes、TestRegression_TaggedTasksReadOnlyWhatTheirTagsSet、TestRegression_NoUntaggedChecks... 未修改且通過。
  • AGENTS.md：§4.4 新增一點（Step 3–5 取 Step 6 的 tag；rescue 只在 detection_engine_binary_result is defined 時還原；「備份 tag == Step 6 tag」由測試鎖住）、新增修訂紀錄 v1.57，並在 v1.41 那列註明 detection-engine 的 Step 3–5 已改。
  • vm-target 實跑（candidate ccde655b，見下方「Actual-run evidence」）：--tags docker 連跑兩次都是 changed=0、沒有備份；同一 snapshot 的 main 對照組是 changed=2 再 changed=1、每次都備份（detection-engine/agent-controller: always-tagged Step 3-5 run when the component isn't selected, so a limited run's state.db backup is changed every time and the idempotency check fails #189 的症狀）。pilot deploy --limit 本身與 prod 沒有實跑，選擇器是直接交給 ansible-playbook site.yml。

service 狀態的還原（歷經三輪嚴格審查後的最終設計）

問題：rescue 第一個 task 無條件 stop 並 disable service。重啟若依賴 C1 tag 才設的 fact（detection_engine_is_upgrade），C1 被跳過時重啟就被略過，健康的 service 會被留在 stopped/disabled（BASE 在這個情境會救回）。審查接著又找到三個更深的問題，都已處理並有測試：

  1. 狀態查詢失敗被吞掉：Step 0 原本 failed_when: false，查詢失敗後 rescue 仍 stop+disable 卻略過還原。
  2. rescue 會重啟進被 Step 10 config validate 拒絕的新 config（只還原 binary/db，沒還原 config）：真 systemd 上重現為 crash loop（NRestarts=5），而「狀態」仍顯示 active/enabled。
  3. 還原時 mode/owner、symlink、密碼出現在 log 的細節。

設計

在 install block 之前（都是 always、唯讀、changed_when: false、check_mode: false）：

  • Step 0：systemctl show --property=ActiveState --property=UnitFileState 記為 detection_engine_service_before。
  • Step 0 assert：要求 rc 為 0、ActiveState= 與 UnitFileState= 各剛好一行且 ActiveState 非空；首次安裝（unit 不存在，show 仍 rc 0 並印空的 UnitFileState=）會通過。任何不符就在 Step 1 之前 fail closed，這個失敗不在 rescue 裡，所以不會觸發 stop/disable。
  • Step 0b/0c/0d：以 stat（get_checksum/get_mime/get_attributes: false）與 slurp（no_log）記下 unit 檔、config.yaml、feature profile、provider.env 的原始內容、mode、數字 uid/gid、是否 symlink 與目標，以及 unit 檔原本是否存在。內容只放在記憶體，沒有備份檔，所以第二次 apply 仍是 changed=0、--check 也能跑。symlink 不會被 slurp（dangling 或連到目錄也不會讓 Step 0c 失敗）。檔案清單是新的 play 變數 detection_engine_rollback_files。

rescue 順序：stop+disable（需 detection_engine_service_before.rc == 0，沒有有效快照就不碰 service）→ 還原 binary/db（守衛不變）→ 還原四個檔（原樣寫回 mode/uid/gid；原本不存在的移除；原本是 symlink 的用 file: state=link 還原）→ daemon-reload → 原本 UnitFileState=enabled 就 re-enable → 原本 ActiveState 為 active/activating/reloading 就 restart。

  • re-enable/restart 要求前面的還原、移除、link 還原、daemon-reload 都沒失敗；任何一步失敗，service 保持停止並 disabled，最後的 fail 訊息會明講是哪一步。這是刻意的 fail closed：寧可停著也不重啟進壞 config。
  • enabled-runtime 不重新 enable（plain disable 不動 /run 的連結，enable 會把它變永久）。inactive、failed、deactivating 維持停止。
  • 首次安裝（unit 原本不存在）：不還原也不移除，service 不動，新檔留著供檢查。
  • 這些 task 的 when 刻意沒有 is defined 與 | default()，讓 repo 的 always-tag lint（AGENTS.md §4.4）自己逼 Step 0 維持 always（改標 C1 會讓 TestRegression_AlwaysTaggedTasksHaveAllPrerequisitesAlways 失敗，已實測）。

行為變更（明講）：失敗後原本 active 的 service 現在會被重啟，而且 config/unit/profile/provider.env 會先被還原。以前（BASE）失敗後會留在 stopped/disabled。rescue 對任何失敗仍會 stop 後再重啟，即使失敗發生在任何變更之前，這是短暫中斷，最終 service 是 active。

驗證

  • 真的 ansible-playbook（ansible-core 2.19.2）+ 假的 systemctl：TestDetectionEngineRescuePutsServiceBack（16 個情境，含 activating、reloading、enabled-runtime、re-enable 失敗但 restart 仍執行、restart 失敗但 enable 已完成）、TestDetectionEngineServiceStateQueryFailsClosed（11 個情境：show 一次性失敗、持續失敗、rc 0 但輸出空/亂/缺行、show 印出合法輸出但 exit 1、systemctl 不存在；都斷言 play 在任何變更前失敗、service 不變、沒有任何 mutating systemctl 呼叫）、TestDetectionEngineRescueDoesNotStopWithoutSnapshot、TestDetectionEngineRescueDoesNotRestartIntoNewConfig（假 unit 在 config 不是舊內容時 crash loop；5 個情境）、TestDetectionEngineStepZeroIsReadOnlyAndCheckModeSafe（apply 與 --check、檔案存在與首次安裝，changed=0）、TestDetectionEngineSnapshotDoesNotLeakProviderEnv（-vvv --diff 與 --check，密碼、檔案 base64、密碼 base64、sha1 都不得出現）、TestDetectionEngineRescueRestoresModeOwnerAndSymlinks、TestDetectionEngineRescueLeavesWhatItWroteWithoutAUnitFile、TestDetectionEngineSnapshotAndRescueHandleUnreadableSymlinks；靜態檢查 rescue 順序、no_log、get_checksum: false、每個 rescue task 都是 always。
  • 真 systemd（user manager）：ExecStart=/bin/sh -c 'grep -q good cfg && exec sleep infinity'、Restart=on-failure、Step 10 拒絕新 config：有修正時前後都是 active/running/enabled、NRestarts=0、config 回到 good；停用還原的對照組是 failed、NRestarts=5。另外六種起始狀態（active+enabled、active+enabled 加一次性 show 失敗、active+enabled-runtime、inactive+disabled、inactive+enabled、active+disabled）失敗後狀態等於失敗前；五種 symlink 情境（連到真檔、dangling、連到目錄、profile 連到 config、unit 檔本身是 symlink）都還原成功。系統層級 systemd（Ubuntu 24.04、systemd 255）的結果見下方「Actual-run evidence」。
  • 變異測試：停用還原、拿掉各個 is not failed 守衛、拿掉 no_log（slurp、還原）、get_checksum 打開、還原 mode/group 改回預設、停用 symlink task、拿掉 unit-existed 守衛、拿掉 stop 守衛、拿掉斷言的各行檢查、拿掉 Step 0 的 changed_when/check_mode、讓 slurp 追 symlink 等，都會被測試抓到。還原 owner 只由靜態測試鎖住（非 root 的測試使用者無法 chown 到別的 uid，生產環境是 root）。
  • 完整的 ./internal/spec 由 CI 的 Go test job 在目前 head 上跑；本機在 candidate 與目前 head 只跑了相關子集（見下方）。
  • 獨立 reviewer 共五輪（每輪都對照 BASE，嚴格以「健康 service 不得被停下、不得洩漏密碼、不得有 ownership/mode 回歸」計 P2）。第五輪是合併 main 與 evidence commit 之後，對目前 head 53f8a8b5 的完整 diff 審查：0 個 P0–P2；以突變確認 F1–F3 的回歸測試會因預期原因失敗；對照原始輸出抽查了 evidence。

已知殘留風險

沒處理的

Actual-run evidence

docs/evidence/detection-engine/2026-10-30-ccde655b4d07.md。candidate 是把 main（fdb6f03e）合併進來的 merge commit ccde655b（tree c39e554b），之後只多了 evidence commit 53f8a8b5（evidence 檔、runbook 兩行、spec 的 ## Actual-run evidence 說明文字；spec 的 yaml checks 區塊在兩者 byte 相同）。兩台全新 Ubuntu 24.04 vm-target（systemd 255）：pr190-de（detection-engine、host-monitoring）、pr190-fx（假的 Thanos Query／Alertmanager）。

  • 正常鏈：L1 通過、L3 ok=116 changed=14 failed=0、L4 ok=118 changed=23 failed=0、L5 12/12、L6 changed=0（同時涵蓋首次安裝）。
  • 以下各情境都從同一個「舊 binary、健康 service」snapshot 起跑，並用同 snapshot、同選擇器、同故障點的 main 做對照：
    • U0 升級：.prev 是舊 binary、pre-upgrade.db 通過 db check、verify 12/12，第二次 changed=0。
    • R189（--tags docker）×2：changed=0 兩次、沒有備份；main 是 changed=2 再 changed=1。
    • Fa（skip C1 + Step 8 失敗）、Fa2（skip C1，在 Step 14 失敗）、Fc1（Step 10 拒絕新 config）、Fc2（Step 11–15 之後在 Step 16 失敗）：都進 rescue；service active/enabled、NRestarts=0、NeedDaemonReload=no；unit／config／profile／provider.env（root:adm 0640）byte、mode、owner 都還原；有升級的情境舊 binary 也回來。main：Fc1 crash loop（failed、NRestarts=5）、Fa2／Fc1 的 provider.env 不見、Fc2 的 unit 與 config 留在這次 run 寫的版本。
    • Fb（Step 0 的 systemctl show 一次性失敗）：在 Step 0 拒絕、不進 rescue；MainPID、ExecMainStartTimestamp 不變。
  • 合成密碼 sentinel 與它的 base64 沒有出現在任何輸出。
  • 限制：故障是注入的；新舊 binary 是同一份 source、不同版本字串（不是跨版本 schema migration）；上游是 fixture；只有 sandbox stage。

本機在目前 head 53f8a8b5：go test -v -run DetectionEngine ./cmd/pilot/cmd/（ansible-core 2.19.2）76 PASS／0 FAIL／0 SKIP；always-tag／rescue／tag-alignment 等 repo lint、spec lint、--syntax-check、golangci-lint 2.12.1（--new-from-rev=origin/main）都通過。

CI 涵蓋範圍的限制（請注意）

本 PR 新增的cmd/pilot/cmd/detection_engine_*_test.go 這幾個 real-ansible 測試 會用真的 ansible-playbook / logrotate 執行，找不到工具時 t.Skip。.github/workflows/ci.yml 的註解寫 Go test job 沒有 ansible-playbook，所以CI 的 Go test 綠燈不一定涵蓋這些測試（CI log 不是 verbose，沒有確認它們在 CI 是跑了還是 skip，見 #198）。這些測試在本機與獨立 reviewer 的環境都實際跑過（目前 head：0 SKIP），結果寫在上面的驗證段；合併後要靠 #198 補上 CI 才有人守著。靜態（YAML 結構）測試則會在 CI 執行。

🤖 Generated with Claude Code

@kjelly @claude
fix(detection-engine): only back up state.db when this run will insta…
9aa0ddb 
…ll the binary

Steps 3-5 are always-tagged, so a --tags run that never reaches Step 6 (pilot deploy --limit
pulls it-core in as a provider host with only the limited hosts' component tags) still took a
'pre-upgrade' backup on every apply while the binary stayed stale. changed_when: true then made
the second apply report a change and fail the idempotency check.

Step 3c now also requires that the run selects Step 6 (all, or one of detection-engine-C1,
detection-engine, observability).

Co-Authored-By: Claude Sonnet 5.5 <[email protected]>
@kjelly

kjelly commented Oct 29, 2026

Copy link
Copy Markdown
Owner Author

Automated PR review: Draft required

Reviewed HEAD: 9aa0ddb3d596163cd922831ad641734de44a3c54
Reviewed Base: d013ed95cf65216a40deefcc2de9c6a4dd57f4ef

Blocking issues / verification notes

  • P2 playbooks/apply/detection-engine-apply.yml:556: With an existing binary whose checksum differs from the artifact, --tags tagged selects Step 6 (tagged detection-engine-C1), but the new hardcoded predicate sees only tagged in ansible_run_tags and sets install_selected/is_upgrade false. Steps 3d-5 skip both DB and previous-binary backups even though Step 6 replaces the binary. A later failure also skips binary/DB restoration and old-service restart because rescue uses is_upgrade. Independent ansible-core 2.19.2 execution of exact extracted HEAD/Base Steps 3-6 and the actual binary-restore task confirms HEAD leaves the new binary with no backups, whereas Base backs up and restores it under identical injected failure. This violates spec section 26 upgrade/rollback requirements. | Fix: Make backup/upgrade gating follow Ansible task-selection semantics, including the special tagged selector and inherited tags, while considering skip-tags; add behavior coverage that requires backup and restoration for every selection that executes Step 6, with tagged as a regression case.
  • Test failed: Independent real Ansible tagged upgrade backup/restore invariant (exact HEAD tasks, injected post-copy failure): /tmp/prm190-independent/head-tagged/run.log
  • Also awaiting: GitHub CI check pending: Go test
  • Also awaiting: Merge state is not CLEAN: 'UNSTABLE'

Next steps: Address blocking findings, rerun affected tests and CI, and request independent review on the updated HEAD/Base before marking Ready.

This comment is an automated technical review, not a human approval.

@kjelly
kjelly marked this pull request as draft October 29, 2026 10:44
@kjelly kjelly mentioned this pull request Oct 29, 2026
Merged
@kjelly @claude
fix(detection-engine): give the backup steps Step 6's tag instead of …
2aad47c 
…a predicate over run tags

The ansible_run_tags predicate missed the special 'tagged' selector and --skip-tags, so
--tags tagged replaced the binary in Step 6 with no backup to restore. Steps 3-5 now carry
Step 6's own tag (detection-engine-C1) rather than always, so Ansible selects them exactly when
it selects Step 6. A real-ansible test covers each selection.

Co-Authored-By: Claude Sonnet 5.5 <[email protected]>
@kjelly kjelly mentioned this pull request Oct 29, 2026
Merged
@kjelly @claude
fix(detection-engine): restore in the rescue only what this run backe…
fd223df 
…d up

The backup (Steps 3-5) now follows Step 6's tag, so the always-tagged
rescue can no longer read detection_engine_is_upgrade with a bare
default(false) (the always-tag prerequisite lint rejects it). The restore
tasks require `detection_engine_binary_result is defined` (this run reached
Step 6) and `detection_engine_is_upgrade is defined and ...`. This also stops
a failing Step 4 from copying a stale .prev/pre-upgrade.db of an earlier
upgrade over the current binary and state.db.

Tests: static guard that Steps 3-5 carry Step 6's tags, real
ansible-playbook rollback tests (9 selections incl. stale backups and a
failing Step 4) and second-apply changed=0 tests. AGENTS.md section 4.4 and
the revision log updated.

Co-Authored-By: Claude Sonnet 5.5 <[email protected]>
@kjelly
kjelly marked this pull request as ready for review October 29, 2026 15:02
This was referenced Oct 29, 2026
Closed
Open
Open
@kjelly

kjelly commented Oct 29, 2026

Copy link
Copy Markdown
Owner Author

獨立審查補充：P2，暫不合併（playbooks/apply/detection-engine-apply.yml:889）。

實測 revision：HEAD fd223df6046c58c7562ff9abc05619bf319ccf86，tree f889e36a3e2dd2bfa989c6fb2be5e39d6e8235c0；BASE d013ed95cf65216a40deefcc2de9c6a4dd57f4ef。使用乾淨隔離 checkout、專用 Ubuntu 24.04 vm-target；未修改 PR source。映像 SHA256：bfab799e2c50a83f77c607e50290646c07628a67ed73fe90c28658a28b0a5394。

正常完整 target test 通過：syntax-check、fresh check-mode（ok=50 changed=8 failed=0）、apply（ok=62 changed=11 failed=0）、committed spec 12 PASS / 0 FAIL / 0 SKIP，第二次 apply ok=60 changed=0 failed=0。這些結果未覆蓋下面的故障回復路徑。

重現條件：原服務 active/enabled，舊 binary 與輸入 artifact checksum 不同。透過 import wrapper 保留 site.yml 的 observability / detection-engine tag 繼承，使用 --tags detection-engine --skip-tags detection-engine-C1；以 detection_engine_feature_profile_dest=/proc/pilot-review190-forced-failure.yaml 令 Step8 複製失敗，當時既有設定尚未變更。HEAD/BASE 都從相同 healthy-old snapshot 起跑，皆預期 apply rc=2。

revision 起始服務 失敗後服務 舊 binary / SQLite
HEAD active / enabled inactive / disabled checksum 不變 / integrity=ok
BASE active / enabled active / enabled checksum 不變 / integrity=ok

原因：C1 被排除後，新的 Step3c tag 不再設定 detection_engine_is_upgrade；rescue 仍停止並停用原服務，新的 restart guard 卻因該 fact 未定義而不恢復服務，造成持續停機。請獨立保存/恢復服務原本狀態，維持備份與 Step6 相同選取，並補上此 failure-path 的實際服務狀態回歸測試。

證據範圍：實際 detection binary、SQLite、systemd；上游採既有 fake-protocol fixtures。兩個 binary 是相同 candidate Go source、不同 version ldflags，沒有宣稱跨版本 DB migration。舊 binary SHA256：d020a3c75def632fcefc48f50ca93b13bafff327015d7e8faa62c7a3662d4908。production apply SHA256：4c040f9c2c225c38bd45405a8a9a3ea1a37715e269e25f807261ad3e45e29c0c；spec SHA256：8577114850903e6d7f43d6199938e1c7b8d5d53e8e9dad8e3be7ecef751a4aaa。

完整本機 evidence：/tmp/prm-184-188-190-20261029-r2/190-vm-evidence-summary.json、190-vm-availability-summary.json、190-target/*-skip-c1-feature-failure.log、190-verification/。公開摘要已排除 secrets；專用測試 VM 已移除，既有 VM 未變更。

@kjelly

kjelly commented Oct 29, 2026

Copy link
Copy Markdown
Owner Author

Automated PR review: Draft required

Reviewed HEAD: fd223df6046c58c7562ff9abc05619bf319ccf86
Reviewed Base: d013ed95cf65216a40deefcc2de9c6a4dd57f4ef

Blocking issues / verification notes

  • P2 playbooks/apply/detection-engine-apply.yml:889: 既有服務 active/enabled、舊 binary 與輸入 artifact 不同時，以繼承 component tag 的 --tags detection-engine --skip-tags detection-engine-C1 執行，Step3c 升級 fact 被跳過，但 Step8 與 rescue 仍會執行。若 Step8 在尚未改動現有設定前失敗，rescue 先停止並 disabled 原服務，新的 restart guard 又因 detection_engine_is_upgrade 未定義而跳過重啟。相同健康 snapshot 的 Ubuntu VM 對照：HEAD 留下 inactive/disabled；BASE 恢復 active/enabled。兩者皆 rc2，舊 binary checksum 不變且 SQLite integrity=ok。這是本次新增的持續停機回歸。 | Fix: 獨立於 C1 升級/備份 fact 保存並恢復服務原本的 active/enabled 狀態；備份仍應與 Step6 同選取。補 component-selected/C1-skipped、後續設定複製失敗且原服務健康的實際 stop/restart 回歸測試。
  • Test failed: Native Ubuntu HEAD-vs-BASE service availability after component-selected/C1-skipped Step8 failure: /tmp/prm-184-188-190-20261029-r2/190-vm-availability-summary.json

Next steps: Address blocking findings, rerun affected tests and CI, and request independent review on the updated HEAD/Base before marking Ready.

This comment is an automated technical review, not a human approval.

@kjelly
kjelly marked this pull request as draft October 29, 2026 15:32
@kjelly @claude
fix(detection-engine): put the service back from its recorded origina…
3bee3d4 
…l state, not from a C1 fact

The rescue stops AND disables pilot-detection-engine.service unconditionally,
but its restart was gated on detection_engine_is_upgrade, a Step 3c fact that
is now C1-tagged and therefore undefined when the selection skips C1 (e.g.
--tags detection-engine --skip-tags detection-engine-C1 with a failure in Step 8):
the service was left inactive/disabled. It also left a healthy service stopped
whenever the binary was up to date. No fact set by a C1-tagged task can serve an
`always` rescue.

New always-tagged, read-only Step 0 records ActiveState/UnitFileState before
anything changes; two always-tagged rescue tasks (re-enable, restart; each
failed_when: false) act on it. The restore tasks keep their binary_result /
is_upgrade guards. The new whens deliberately have no `is defined` guard, so the
always-tag prerequisite lint itself keeps Step 0 `always`.

Behaviour change: after a failure in a non-upgrade run the rescue now restarts a
service that was active (before, a failure after a config change left it stopped
and disabled); the rescue does not restore config/unit files, so a bad new
config means the service restarts with it. Also reword the Step 3 comment about
the backups (known exceptions: #193, #194).

Tests: TestDetectionEngineRescuePutsServiceBack (real ansible-playbook, fake
systemctl, real Step 0, Steps 3-6 and rescue; 11 scenarios) and static guards
for Step 0 and the service-state rescue tasks.

Co-Authored-By: Claude Sonnet 5.5 <[email protected]>
@kjelly kjelly mentioned this pull request Oct 29, 2026
Open
@kjelly
kjelly marked this pull request as ready for review October 29, 2026 18:49
@kjelly

kjelly commented Oct 30, 2026

Copy link
Copy Markdown
Owner Author

Automated PR review: Draft required

Reviewed HEAD: 3bee3d4539d4cd812f4eba4b2af40245aec2434a
Reviewed Base: d013ed95cf65216a40deefcc2de9c6a4dd57f4ef

Blocking issues / verification notes

  • P2 playbooks/apply/detection-engine-apply.yml:514: Step0 用 failed_when:false 吞掉 systemctl show 的所有錯誤，但新的 re-enable/restart 都要求該查詢 rc==0。原服務 active/enabled、舊 binary 需要升級時，若狀態查詢失敗而後續 task 再失敗，rescue 仍停止並 disable 服務，還原 binary/DB 後卻跳過恢復服務。獨立 real-Ansible 的一次性 query-error 對照：HEAD 留 inactive/disabled；BASE 恢復 active/enabled。這是錯誤注入測試，沒有宣稱觀察到自然發生的 D-Bus 故障。 Root 另以乾淨精確 HEAD 在專用 Ubuntu24.04 VM 從相同 healthy snapshot 對照：僅讓 Step0 的 show 一次性 rc1，其餘呼叫交給真實 /usr/bin/systemctl；Step8 複製失敗後 HEAD inactive/disabled，BASE active/enabled，兩者 rc2、舊 binary SHA 不變、SQLite integrity=ok。正常全鏈則 12/12、第二次 apply changed0；一般3種失敗回復皆通過，仍未涵蓋此 query-error。 | Fix: 在任何 mutation 前取得有效狀態快照；查詢失敗需 fail closed，且此拒絕本身不能觸發無條件 stop/disable（可將必需探測移出 mutation block，或用有效快照守住 rescue mutation）。保留 unit 不存在但 show rc0 的首次安裝情境；補一次性 show 失敗的回歸測試。
  • Test failed: Independent failed-service-snapshot rollback availability invariant, exact HEAD/Base extracted production tasks and real ansible-core2.19.2: /tmp/prm-184-188-190-20261030/190-independent-probe/summary.json
  • Test failed: Root native systemd HEAD/BASE one-shot query-error availability invariant: /tmp/prm-184-188-190-20261030/190-native-probe-error.json

Next steps: Address blocking findings, rerun affected tests and CI, and request independent review on the updated HEAD/Base before marking Ready.

This comment is an automated technical review, not a human approval.

@kjelly
kjelly marked this pull request as draft October 30, 2026 00:07
kjelly and others added 4 commits October 30, 2026 12:21
@kjelly @claude
fix(detection-engine): fail closed before any change when the service…
6c78bf3 
…'s original state cannot be read

Step 0 sat inside the install block with failed_when: false. A failed
`systemctl show` (one-shot rc 1, unreadable output, no systemctl) was swallowed,
yet the rescue's first task stops AND disables the unit unconditionally and its
re-enable/restart tasks need that query's rc == 0. So with an active/enabled
service and a later failing task (e.g. Step 8) the rescue left a healthy service
inactive/disabled with nothing to put it back; before this PR's rescue changes
the same run restarted it.

Step 0 is now two always-tagged tasks BEFORE the block: the read-only probe, and
an assert (rc 0, one non-empty ActiveState=, one UnitFileState=) that fails the
play before Steps 1-17 and outside the rescue. A first install still passes
(`show` exits 0 for a missing unit). The rescue's stop+disable also requires
detection_engine_service_before.rc == 0, so even with no snapshot at all it does
not touch the service (the `when` on the undefined register fails the rescue first);
no `is defined`, so the always-tag lint keeps pinning Step 0.

Restore semantics: activating/reloading are restarted like active; enabled-runtime
is not re-enabled (a plain `disable` leaves a /run link alone, verified against a
systemd user manager, and `enable` would make it persistent). The re-enable/restart
tasks use ignore_errors + register instead of failed_when: false and the final
message says when either restore FAILED.

Tests: TestDetectionEngineServiceStateQueryFailsClosed (10 query-failure cases:
one-shot and persistent show failure, empty/garbage/partial output, no systemctl;
asserts the run fails with the Step 0 refusal, the unit is unchanged, no mutating
systemctl call, nothing backed up or replaced), TestDetectionEngineRescueDoesNotStopWithoutSnapshot,
and 5 new restore scenarios (activating, reloading, enabled-runtime, failing
re-enable, failing restart). Static test pins Step 0 outside the block and the
stop guard.

Co-Authored-By: Claude Sonnet 5.5 <[email protected]>
@kjelly @claude
fix(detection-engine): restore the unit/config files before the rescu…
f44f334 
…e restarts the service

Steps 8/9/9b/9c/11 rewrite the feature profile, config.yaml, provider.env and the
unit file before Step 10's `config validate` and the health checks can reject
them. The rescue restored only the binary and state.db and then restarted the
service, so a healthy active service was restarted into the config Step 10 had
just rejected (Restart=on-failure unit: crash loop, while a state check still
says active; reproduced on a systemd user manager).

Three always-tagged read-only tasks before the install block (Step 0b stat, 0c
slurp with no_log, 0d unit-existed fact) hold the files' original contents in
memory (list: play var detection_engine_rollback_files, unit path:
detection_engine_unit_path). No backup files, so nothing goes stale and a second
apply stays changed=0; fine under --check. The rescue now runs stop -> binary/db
-> restore files (originals back, files that did not exist removed) -> daemon
reload -> re-enable -> restart, only when the unit already existed (a failed first
install leaves the files for inspection). The restore tasks use ignore_errors +
register and re-enable/restart require them not failed: if the old files cannot be
put back the service stays stopped and the final message says so, rather than
restarting into a config this run wrote.

Tests: TestDetectionEngineRescueDoesNotRestartIntoNewConfig (fake unit that
crash-loops unless config.yaml has the old content: Step 10 rejects, failure after
the unit was rewritten + daemon-reload, provider.env created by the run, first
install, files that cannot be put back), TestDetectionEngineStepZeroIsReadOnlyAnd
CheckModeSafe (changed=0, ok/skipped counts under apply and --check), Step 0* in
the second-apply changed=0 test, a case where `show` exits 1 with valid-looking
output (pins the rc assert), static order/gate checks.

Co-Authored-By: Claude Sonnet 5.5 <[email protected]>
@kjelly @claude
fix(detection-engine): keep the config snapshot out of the logs and r…
78673c5 
…estore files as they were

P3 follow-ups from the review of the rollback snapshot:

- Step 0b's stat no longer computes a checksum (nothing uses it and -vvv printed
  the sha1 of provider.env); mime/attributes off too. The slurp and the restore
  stay no_log. A static test pins no_log on the slurp and get_checksum: false,
  and a -vvv --diff test asserts the secret, its base64 and the file's sha1 never
  appear in the output.
- The restore uses the original file's mode and numeric owner/group from the
  stat (not the playbook's defaults), and a file that was a symlink comes back as
  the same symlink to its lnk_target (the run's copy replaced it with a regular
  file; symlinks are not slurped). detection_engine_rollback_files no longer
  carries modes. The re-enable/restart gates include the new links task.
- Tests: 0640 provider.env with a secondary group, 0600 config.yaml and 0664
  profile come back with the same mode/gid; a symlinked config.yaml comes back as
  the link with its target untouched; unit file absent with config.yaml and
  provider.env present and a failure after Step 11 leaves them as the failed run
  wrote them and the service untouched (the unit-existed guard).

Co-Authored-By: Claude Sonnet 5.5 <[email protected]>
@kjelly @claude
test(detection-engine): pin that the config snapshot skips symlinks i…
3b2538a 
…t cannot read

Step 0c slurps only `not islnk` files because slurp follows links: a dangling
config.yaml link or a link to a directory would fail the snapshot. The existing
symlink test used a link to a regular file, which slurp reads fine, so removing
the `not item.stat.islnk` guard went unnoticed. New subtests (dangling link, link
to a directory; failure injected after Step 11) assert the snapshot does not fail,
the rescue recreates the link with the same target, the target is untouched, the
other files are restored and the service ends in its original state. They fail
with a slurp that follows links. Test files only.

Co-Authored-By: Claude Sonnet 5.5 <[email protected]>
@kjelly kjelly mentioned this pull request Oct 30, 2026
Merged
@kjelly @claude
test(detection-engine): satisfy staticcheck QF1001 in the rescue orde…
3d17970 
…r check

CI's golangci-lint v2 flags the negated chain of comparisons; write the
condition without the negation. No behaviour change.

Co-Authored-By: Claude Sonnet 5.5 <[email protected]>
@kjelly
kjelly marked this pull request as ready for review October 30, 2026 10:15
kjelly added a commit that referenced this pull request Oct 30, 2026
@kjelly
fix(detection-engine): bind rollback to verified current-run snapshots
5ebc8cb 
Refs #193, #194. Includes PR #190 compatibility baseline without modifying its branch.

Use private per-apply snapshots, fail closed before Step 6 on backup or validation errors, revalidate rollback sources and stop before SQLite/WAL restoration. Add isolated vm-target fault-injection CI with real SQLite CLI and systemd.

Scan: agent-controller retains a similar shared-backup pattern; intentionally outside this detection-engine issue pair.
@kjelly kjelly mentioned this pull request Oct 30, 2026
Merged
@kjelly

kjelly commented Oct 30, 2026

Copy link
Copy Markdown
Owner Author

Automated PR review: Draft required

Reviewed HEAD: 3d1797024f34e537239540b51e9a1cf15e3c5acc
Reviewed Base: d013ed95cf65216a40deefcc2de9c6a4dd57f4ef

Blocking issues / verification notes

  • P1 playbooks/apply/detection-engine-apply.yml:515: This candidate changes service-state probes and unit/config/profile/provider.env rollback plus tag-selected upgrade behavior, but contains no committed candidate-bound target execution record. Required repository acceptance evidence is missing. | Fix: Freeze a candidate and run the sanctioned fresh syntax/check-diff/apply/committed-spec verification/second-apply changed=0 chain on a dedicated disposable Ubuntu 24.04 sandbox vm-target, recording inventory/stage facts, candidate/tree/file hashes and target image digest. Also execute the relevant failure matrix: inherited component tag with C1 skipped and Step8 failure; one-shot service-query failure before install-block mutation; rejected new config and failure after unit rewrite, asserting native availability and restored unit/config/profile/provider.env (plus binary hash/DB integrity where upgraded). Commit a sanitized docs/evidence/detection-engine record via evidence-only commit and prove reviewed content equals the tested candidate. No production run is required.

Next steps: Address blocking findings, rerun affected tests and CI, and request independent review on the updated HEAD/Base before marking Ready.

This comment is an automated technical review, not a human approval.

1 similar comment
@kjelly

kjelly commented Oct 30, 2026

Copy link
Copy Markdown
Owner Author

Automated PR review: Draft required

Reviewed HEAD: 3d1797024f34e537239540b51e9a1cf15e3c5acc
Reviewed Base: d013ed95cf65216a40deefcc2de9c6a4dd57f4ef

Blocking issues / verification notes

  • P1 playbooks/apply/detection-engine-apply.yml:515: This candidate changes service-state probes and unit/config/profile/provider.env rollback plus tag-selected upgrade behavior, but contains no committed candidate-bound target execution record. Required repository acceptance evidence is missing. | Fix: Freeze a candidate and run the sanctioned fresh syntax/check-diff/apply/committed-spec verification/second-apply changed=0 chain on a dedicated disposable Ubuntu 24.04 sandbox vm-target, recording inventory/stage facts, candidate/tree/file hashes and target image digest. Also execute the relevant failure matrix: inherited component tag with C1 skipped and Step8 failure; one-shot service-query failure before install-block mutation; rejected new config and failure after unit rewrite, asserting native availability and restored unit/config/profile/provider.env (plus binary hash/DB integrity where upgraded). Commit a sanitized docs/evidence/detection-engine record via evidence-only commit and prove reviewed content equals the tested candidate. No production run is required.

Next steps: Address blocking findings, rerun affected tests and CI, and request independent review on the updated HEAD/Base before marking Ready.

This comment is an automated technical review, not a human approval.

@kjelly
kjelly marked this pull request as draft October 30, 2026 14:42
kjelly and others added 2 commits October 30, 2026 23:06
@kjelly @claude
Merge origin/main (fdb6f03) into fix/detection-engine-always-backup
ccde655 
Co-Authored-By: Claude Opus 5.5 <[email protected]>
@kjelly @claude
docs(evidence): record the vm-target run for #190 on ccde655
53f8a8b 
Candidate ccde655 (merge of origin/main fdb6f03 into the PR head) on two
fresh vm-targets: normal chain L1/L3/L4, C1-C12 12/12 and L6 changed=0, first
install accepted. Backup selection follows Step 6 (--tags docker twice:
changed=0, no backup), the upgrade backups are the old binary and a database
that passes db check, an unreadable Step 0 state is a refusal without restart,
and four rescue arms put the service and the unit/config/profile/provider.env
back as found. The same arms on main reproduce #189, a crash loop, and
unrestored unit/config/provider.env. Observation not fixed here: Step 14 reads
an undefined register when Step 6 is not selected (same on main).

Co-Authored-By: Claude Sonnet 5.5 <[email protected]>
kjelly added a commit that referenced this pull request Oct 30, 2026
@kjelly
docs(evidence): reconcile PR #190 verification records without changi…
af9259a 
…ng the tested runtime

Merge PR #190 evidence-only follow-up 53f8a8b. Preserve both evidence records and resolve their verification index additions. Runtime, test driver, fixtures and verification rows are unchanged from tested candidate 5adea80 and all-green CI at 5265084.
@kjelly
kjelly marked this pull request as ready for review October 30, 2026 16:40
@kjelly kjelly mentioned this pull request Oct 31, 2026
Open
@kjelly
kjelly merged commit 37bdcc8 into main Oct 31, 2026
7 checks passed
@kjelly
kjelly deleted the fix/detection-engine-always-backup branch October 31, 2026 02:22
This was referenced Nov 1, 2026
Open
Closed
Merged
kjelly added a commit that referenced this pull request Nov 3, 2026
@kjelly @claude
test(spec): lint always-tagged tasks that report changed on every run (…
260ae89 
…#189)

#189: detection-engine Steps 3-5 were `always`-tagged and
Step 4 was `changed_when: true`, so a `pilot deploy --limit` run that only
pulled the host in through dependency expansion changed it on every apply
and the second-apply changed=0 check failed forever. #190/#203 re-tagged
those steps with Step 6's tag, locked by a per-file test. No repo-wide
check covers the class: with the pre-#190 tags put back, the five existing
tag lints (AlwaysTaggedTasksHaveAllPrerequisitesAlways,
RescueRunsWheneverItsBlockDoes, TaggedTasksReadOnlyWhatTheirTagsSet,
PreTaskGatesRunUnderApplyTags, NoUntaggedChecksInTaggedPlays) all passed.

TestRegression_AlwaysTaggedTasksConverge scans playbooks/apply/*.yml and
the task files they include (include_tasks children get only `apply` tags
and need an always include line; import_tasks children inherit the line's
tags; rescue is skipped, block `always:` sections are checked). An
always-run command/shell/raw/script task must declare a changed_when that
is not literally true, a tempfile task `changed_when: false`. At HEAD it
finds exactly two, in a ratchet allowlist with reasons and a stale-entry
check: agent-controller Step 4 (same shape, tracked by #195) and
freeipa-realm-replacement's one-off trust snapshot. The walk fails closed
on what it cannot follow (roles, import_playbook, action:, merge keys, an
unreadable or templated always-run include) and must examine a minimum
number of tasks, so it cannot pass by finding nothing.

TestFindAlwaysTaggedNonConvergent covers the tag rules on synthetic
playbooks; TestAlwaysTaggedConvergenceFlagsThePre190Playbook puts the
pre-#190 tags back on the real detection-engine playbook in memory.

Also fix three ways TestDetectionEngineSecondApplyChangedZero could not
catch #189: the run that does not select Step 6 never asserted changed=0;
its "no backup taken" check looked at backups/pre-upgrade.db although
Step 4 has written to backups/apply-*/ since #203; and the Step 3-6 task
regex missed Steps 3e, 4e, 4f and 5e (now checked against the real task
names). No playbook changes.

Co-Authored-By: Claude Fable 5.1 <[email protected]>
kjelly added a commit that referenced this pull request Nov 3, 2026
@kjelly @claude
docs(agents): restore the #190 §4.4 rule lost in the #203 merge and c…
b71dfce 
…ite the new lint (#189)

that replaces the thing, never `always`) and a changelog row; the #203 merge
deleted both. v1.41's row still said detection-engine's Step 3-5 are tagged
`always` for the rescue, with no correction.

Restore the point as a class-level rule: an `always` task must not report
changed on every run, because a `pilot deploy --limit` run runs every
`always` task of every imported play on a dependency-expanded host. It
describes the rollback mechanism as it is after #203 (the always-reset
detection_engine_rollback fact, granted by Step 6's tag after the snapshot
validates), not #190's `_binary_result is defined` wording, and cites the
new lint TestRegression_AlwaysTaggedTasksConverge with its two allowlist
entries (agent-controller Step 4 for #195; freeipa-realm-replacement's
one-off trust snapshot). The rescue bullet that says snapshot/upgrade
facts must be `always` now points at the exception. v1.41's row gets a
correction and v1.58 records the change. No docs/runbooks or
docs/verification text said Step 3-5 are `always`; the evidence records
are left as written.

Co-Authored-By: Claude Fable 5.1 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@kjelly