[mikomiel]

Summary

• add deploy/ci-burst-state.py and run it from ci-burst-refresh.service as ExecStopPost, so every refresh attempt writes /var/lib/ci-burst-image/deployment-state.json;
• record the deployed and channel commits, image ID, last success, pinned state and unit drift against the deployed commit;
• add a hold file to freeze promotion without stopping observation, and document rollback by retagging the previous image before writing the hold;
• install the new script in both the controller and cleanup-host procedures.

The document is a plain deployment-state.v1 JSON file for an external metrics collector. It contains no credential, host address or private path, and nothing is published to a registry or announced as a release.

Tests

python3 -m unittest discover -s tests -p 'test_*.py'
Ran 764 tests in 40.576s
OK (skipped=2)

The new tests cover current, behind, held and failed documents, missing artifacts, and SHA-file validation. The unit contract pins ExecStopPost and the hold guard.

Deployment and rollback

This is source only: no host has been changed by this pull request. The new script and unit line require the documented install step, and the hold file is the rollback handle. Revert the pull request to withdraw the producer; the previous units keep working.

[ecomwizards-gandalf]

Independent snapshot review of afaeda301c19...79584d69b973 in mikomiel/ci-burst by pi on codex-lb (gpt-6.1-sol).

Verdict: REQUEST_CHANGES

Reviewer output

Findings

1. [P1] The default document exposes a host identifier.
   Location: deploy/ci-burst-state.py:142
--target defaults to the machine’s short hostname, and the installed ExecStopPost supplies no override. Every normal refresh therefore copies a real estate identifier into the collector-facing document. Removing the DNS suffix does not anonymize it. This violates the requested public-content boundary and contradicts the documentation’s assurance. Use an explicitly configured, public-safe logical target; do not derive it from host identity. Add a test proving that the default invocation cannot emit the hostname.

2. [P1] Rollback leaves the producer reporting the wrong deployed source and prevents the promised resume.
   Locations: deploy/ci-burst-state.py:149–160; docs/container.md:420–425
   After deploying commit B, the documented rollback retags image A as latest but leaves deployed-sha at B. The producer independently reads B from that file and A’s image ID from Podman, without checking their association. With main still at B, it reports B’s version, behind: false, integrity_ok: true, and drift against B—even though A serves. A held refresh also advances last_success.

   Removing the hold does not restore B: ci-burst-refresh.sh:98–100 sees recorded B equal channel B and exits without promotion. Reconcile the serving image and deployed-source record during rollback, represent unknown provenance honestly, and ensure unholding can restore the channel image. Test the complete deploy → rollback → observe → unhold sequence.

3. [P1] The new rollback recipe can freeze the image it was meant to withdraw.
   Location: docs/container.md:420–425
   The two commands neither stop an active refresh nor acquire refresh.lock. If a refresh has already passed the hold check at deploy/ci-burst-refresh.sh:93, the operator can retag the previous image and write the hold, after which that in-flight refresh promotes its candidate anyway. Subsequent attempts then honor the hold and preserve that candidate.

   The existing rollback procedure at docs/container.md:619–628 explicitly addresses this race, but the newly added standalone recipe bypasses it. Provide one consistent procedure that quiesces refresh and performs rollback plus hold creation under the refresh lock before observation resumes.

4. [P2] The standard controller installation never compares any units.
   Location: deploy/ci-burst-state.py:140,159–160
   The producer defaults CI_BURST_UNITS to an empty string. Neither the base service nor the controller installation sets it. The default unit list in ci-burst-refresh.sh:56 is only a shell variable in the separate ExecStart process; it cannot reach ExecStopPost. Consequently, the standard controller document always contains drift: [], including when an installed unit is missing or changed. The cleanup drop-in happens to supply its own list.

   Give the producer the same controller defaults and preserve role-specific overrides. Test the default producer invocation with both changed and missing installed units.

5. [P2] Failed channel observations are stamped as freshly checked.
   Locations: deploy/ci-burst-state.py:99,150–156
   When fetching fails, the producer can fall back to an old origin/main or remaining FETCH_HEAD, but it always publishes source_checked_at: now. During a sustained outage, the document therefore keeps presenting an old channel SHA with a fresh observation timestamp. Although result reports failure, the source freshness assertion is false.

   Persist the channel SHA and its observation time only after a successful fetch, independently of promotion. On failure, retain that timestamp or explicitly mark freshness unknown. Test a successful observation followed by a failed fetch.

Test coverage and suggestions

• tests/test_deployment_state.py tests build_document() with supplied values and tests _sha(), but never exercises main(), _drift(), publication, or actual preservation of a previous document’s last_success. For example, the failed-attempt test supplies last_success=10 and verifies that the renderer copies it; it does not test the retention logic.
• Add credential-free behavioral tests using temporary repositories and fake Podman commands for the findings above, plus atomic publication and failed artifact inspection.
• _drift() also silently skips an unreadable deployed-source unit (deploy/ci-burst-state.py:65–67). Distinguish “comparison unavailable” from “no drift” rather than publishing an apparently clean list.

Review scope and limitations

• Read: the complete supplied diff, all changed files, and supporting deployment, security, contribution, and test sources.
• Tests/checks: UNAVAILABLE: no command-execution tool was provided. I did not independently verify the claimed 764-test run.
• Material unavailable: the snapshot has no .git history, so I could not independently verify the range or inspect its commit messages. The external collector implementation/schema was not available for compatibility review.

VERDICT: REQUEST_CHANGES

Evidence: not written. This review is evidence, not merge authorization.

[ecomwizards-gandalf]

Independent snapshot review of afaeda301c19...97be9dcc79fc in mikomiel/ci-burst by pi on codex-lb (gpt-6.1-sol).

Verdict: REQUEST_CHANGES

Reviewer output

Finding

[P2] Failed-fetch freshness is still inferred from FETCH_HEAD when the success marker is absent — deploy/ci-burst-state.py:190–196

The refresh script correctly publishes source-sha only after a successful fetch. However, when that file is absent—as on the first run after installing this change—the producer falls back to FETCH_HEAD and uses its mtime without checking whether the fetch succeeded.

Git can populate FETCH_HEAD and nevertheless return nonzero, for example when updating refs/remotes/origin/main fails because its ref lock cannot be acquired. In that case:

1. Refresh exits at deploy/ci-burst-refresh.sh:83–86 without publishing source-sha.
2. ExecStopPost receives a failed SERVICE_RESULT.
3. The fallback nevertheless reports the newly written FETCH_HEAD and its fresh timestamp as source_checked_at.

The document says result: failed, but still presents a fresh channel check, violating the requested failed-fetch freshness invariant.

Requested fix: Treat the successfully published observation marker as authoritative. Without one, retain a previously established observation or report unknown freshness rather than deriving it from bookkeeping that a failed fetch can modify. Add a regression covering an absent marker, a readable freshly modified FETCH_HEAD, and a failed service result. This failure path was identified by source inspection, not execution.

Re-review of the five requested findings

1. Hostname-derived target: addressed in source.
   deploy/ci-burst-state.py:25,180 uses a logical controller default and explicit configuration, not the hostname. The cleanup drop-in supplies reaper.

2. Serving revision and rollback reporting: addressed in source.
   deploy/ci-burst-state.py:197–201 obtains deployment identity from the image labels and does not fall back to deployed-sha. The refresh build supplies those labels.

3. Rollback quiescence and hold: addressed in the documented primary procedure.
   docs/container.md:631–636 stops scheduling and the service, then retags and writes the hold within the refresh lock before restarting the timer. The refresh script fetches before checking the hold and skips promotion when held.

4. Controller unit list: addressed in source.
   deploy/ci-burst-refresh.service:29 supplies all six controller service/timer names to both processes. The cleanup drop-in overrides the list.

5. Failed-fetch freshness: not fully addressed, because of the fallback above. The normal path with an existing valid source-sha preserves its timestamp.

Optional improvements

• Add behavioral regressions for the repaired paths.
  tests/test_deploy_units.py:236–256 checks text presence, not hold/unhold execution or fetch ordering. The behavioral harness now supports a serving revision, but no test supplies a mismatching revision to exercise rebuilding after rollback. No test calls the producer’s main() to verify label selection, observation timestamps, environment wiring, or the written JSON. Add:

  • held refresh fetches but neither builds nor promotes;
  • removing the hold rebuilds when deployed-sha matches the channel but the serving label differs;
  • failed fetch preserves the published observation;
  • rollback state reports the serving label rather than the recorded SHA.

• Repair the interrupted networking paragraph at docs/container.md:451–458; the new cleanup-target paragraph splits “see the networking paragraph below.”

Review and validation limits

• Read the complete supplied diff and relevant implementation, tests, deployment documentation, and repository standards.
• The PR title follows Conventional Commits, and the changed content contains no apparent credential values or new publication surface.
• Tests: NOT RUN. Available tools support file inspection only; I did not independently validate the claimed 764-test result.
• No required source material was unreadable after resolving the repository’s absolute path. Commit history/messages were not available in the supplied snapshot, so their convention compliance was not verified.

VERDICT: REQUEST_CHANGES

Evidence: not written. This review is evidence, not merge authorization.

[ecomwizards-gandalf]

Independent snapshot review of afaeda301c19...8992d051aa79 in mikomiel/ci-burst by pi on codex-lb (gpt-6.1-sol).

Verdict: APPROVE

Reviewer output

Findings

No blocking findings in the supplied diff. The remaining P2 is resolved at source level.

Focused re-review

Area	Source-review result
Missing observation marker	deploy/ci-burst-state.py:85–94 reads only source-sha. When absent, it returns ("", 0); neither FETCH_HEAD nor origin/main supplies either field.
Successful publication	deploy/ci-burst-refresh.sh:83–92 exits on fetch failure and publishes the marker by rename only after successful fetch and resolution. A failed fetch therefore cannot advance the marker’s observation time.
Producer integration	deploy/ci-burst-state.py:192–208 carries the marker-derived values directly into the document. Other Git reads are for deployed-version lookup and unit drift, not channel freshness.
Regression test	tests/test_deployment_state.py:106–114 asserts the exact absent-marker result despite an existing FETCH_HEAD, then asserts that a published marker supplies the SHA. This tests behavior rather than merely matching source text.
Documentation	docs/container.md:422–425 accurately describes unknown freshness until the marker exists. The existing host-path table remains a container-access table; the new deployment-state artifacts are host-local, so their absence from that table is not contradictory.

The related safeguards remain present and consistent:

• Logical controller/reaper targets, rather than hostname defaults.
• Serving-image revision labels as deployed identity, including rebuilding after a rollback when the hold is removed.
• Conditional advancement of last_success, preserving it on failed attempts.
• Explicit drift entries when source content or installed units are unavailable, with role-specific unit lists.
• Rollback instructions that stop scheduling and the service, then retag and create the hold under the refresh lock.

Both installation procedures include the producer. The changed content introduces no secret values or registry/release publication. The PR title follows Conventional Commits; commit subjects were not available to verify separately.

Optional suggestions

1. Repair the interrupted paragraph — docs/container.md:454–461: the new cleanup-target paragraph splits “see the networking paragraph below.” Move it after the complete networking paragraph. This is a readability issue, not a deployment blocker.

2. Strengthen observation-time coverage — tests/test_deployment_state.py:106–114: set a known marker mtime and assert the complete (SHA, timestamp) tuple. A behavioral fetch-failure test preserving an existing marker and its mtime would also protect the producer/writer contract.

Validation and limits

• Read: the complete supplied diff, all changed sections, and relevant surrounding source and repository standards.
• NOT RUN: unit tests, systemd/Podman checks, secret scanning, or live deployment validation; only read/search tools were available. The PR’s reported test result is not independently verified.
• No portion of the supplied diff remained unread. Earlier review text was not supplied, so the safeguard confirmation above is against the current source rather than earlier finding IDs.

VERDICT: APPROVE

Evidence: not written. This review is evidence, not merge authorization.