From feb4b37a20617f471e29fff643a9259a0f6226f5 Mon Sep 17 00:00:00 2001 From: Xore Date: Sun, 27 Sep 2026 15:00:25 +0200 Subject: [PATCH 1/2] docs(ops): document the #3315 revision contract, and correct it after the health split MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #3315 stamped both dashboard images with their git revision and exposed it from /healthz. The stamping is in main and works; what did not survive is the documentation of it, because the health endpoint was refactored underneath it. Two stale facts, both now wrong against the code: - docs/OPERATIONS.md's "Service health contract" section showed the /livez body as {"live":…,"built":…} with no revision at all. That is the one page an operator opens to learn what these endpoints answer, and the field this issue exists to add was missing from it. - docs/ARCANE-GIT-SYNC.md showed /healthz as {"ok":true,"es":true, "revision":…}. #3317 split the endpoint into /livez (no Elasticsearch) and /readyz (503 + reason), so ok and es no longer exist. The doc described a response shape the service stopped returning a refactor ago, which is worse than saying nothing: an operator greps for it and concludes the stamp is broken. Adds the revision contract to OPERATIONS.md where the task asked for it -- the four surfaces (health body, boot log, /build.json, image label), where GIT_SHA comes from, what "unknown" means and why it is the honest default, how to turn it on, and verify-deploy.sh's three-valued exit codes. Also states why /readyz deliberately carries no revision: readiness is a property of the running container, not of a build, and the two answers diverge during a rollback. Verification (this branch, no code changed): cargo test 549 passed, 0 failed, 1 ignored cargo run --bin openapi | diff openapi.json byte-identical, no drift openapi.json 132 paths; /healthz {200,502}, /livez {200}, /readyz {200} cargo build --release ok (GIT_SHA stamped in) Live, against a release binary built with GIT_SHA=$(git rev-parse HEAD): /healthz 200 {"live":true,"built":"…","revision":"17c7e412adfb…"} /livez 200 (identical body) /readyz 503 {"ready":false,"reason":"elasticsearch is unreachable: …"} That last pair is the constraint the task named: /livez answered 200 with Elasticsearch pointed at a dead port, so it stays ES-independent and cannot restart-loop the container, while /readyz 503s with a reason. verify-deploy.sh driven end to end against that live process: 0 on a matching revision, 1 on a stale one, 0 on an abbreviated `git rev-parse --short HEAD` (a reported abbreviation, not a cry-wolf), and 2 — not 1 — when the endpoint cannot be read, so an unreadable body can never read as a stale deploy. Same for the frontend's /build.json form. Image labels exercised for real: the runtime-stage ARG/LABEL block was extracted from both Dockerfiles and built against busybox, stamped and unstamped. Stamped -> the real revision; unstamped -> "unknown" for both revision and created, never a fabricated value. scripts/tests/test_3315_image_revision.py: 40 passed, 48 subtests. The four docs gates (paths-exist, reachable, stale-paths, compose-env) pass. Refs #3315 Co-authored-by: OpenCode --- docs/ARCANE-GIT-SYNC.md | 8 +++- docs/OPERATIONS.md | 104 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 109 insertions(+), 3 deletions(-) diff --git a/docs/ARCANE-GIT-SYNC.md b/docs/ARCANE-GIT-SYNC.md index 7ba46f1e..43fc3c15 100644 --- a/docs/ARCANE-GIT-SYNC.md +++ b/docs/ARCANE-GIT-SYNC.md @@ -518,8 +518,12 @@ carry the git revision they were built from, in three places, and - **Backend** — compiled in. `backend-service/build.rs` reads the `GIT_SHA` build arg and re-exports it as `APIARY_GIT_SHA`; `/healthz` returns it as - the `revision` field (`{"ok":true,"es":true,"revision":"3dca445…"}`) and - the boot log line carries it. `build.rs` also emits + the `revision` field of the liveness body + (`{"live":true,"built":"2026-09-27T01:46:34+00:00","revision":"3dca445…"}`) + and the boot log line carries it. Note the `ok`/`es` pair that used to sit + beside it is gone: #3317 split the endpoint into `/livez` (no Elasticsearch) + and `/readyz` (503 + `reason`), so `revision` now rides on the + liveness side. `build.rs` also emits `cargo:rerun-if-env-changed=GIT_SHA`, which is load-bearing: without it cargo reuses a cached build and the second build of an identical tree keeps the *first* revision, producing the exact failure this fixes. diff --git a/docs/OPERATIONS.md b/docs/OPERATIONS.md index c85a4338..8f8803df 100644 --- a/docs/OPERATIONS.md +++ b/docs/OPERATIONS.md @@ -249,7 +249,7 @@ appeared there. Split into: ```console $ curl -s http://backend-service:8081/livez -{"live":true,"built":"2026-09-27T01:46:34+00:00"} +{"live":true,"built":"2026-09-27T01:46:34+00:00","revision":"3dca4457f1b2c0d4e5a69788796a5b4c3d2e1f0ab"} $ curl -s http://backend-service:8081/readyz {"ready":true,"cluster":"green","write_blocked":[]} @@ -258,6 +258,12 @@ $ curl -s -o /dev/null -w '%{http_code}\n' http://backend-service:8081/readyz 503 ``` +`/readyz` deliberately carries **no** `revision`: readiness is about whether +this process can do its job, which is a property of the running container and +not of a build, and the two questions have different answers during a rollback. +The revision belongs to the liveness body, which is the one every probe and +every ops script already reads. + **`/livez` is what the container `HEALTHCHECK` curls, and it must stay that way.** A probe that can block on Elasticsearch converts that dependency's outage into a restart loop of a container that was never the problem. The @@ -302,6 +308,102 @@ that blocks for 30s is indistinguishable from the outage it exists to report. - **"Why is the dashboard empty?"** → `/api/v1/source-health` (the per-sensor freshness page). `/readyz` says the backend cannot write; only source-health says whether events are arriving. +- **"Which commit is actually deployed?"** → `revision` on `/livez`, plus + the image label. See below. + +### Which revision is deployed (#3315) + +The failure this exists for is the one `docs/ARCANE-GIT-SYNC.md` names +directly: a content-change redeploy leaves the dashboard **"green, healthy, +running the old code."** A fresh container id, a passing healthcheck and a +recent image build all say the machinery ran — none of them say this code is +what is running. So both dashboard images carry the git revision they were +built from, and `scripts/verify-deploy.sh` compares it against what you +expect. + +**The contract, in one table.** Four surfaces, one value: + +| Surface | Where | Read it with | +|---|---|---| +| Backend health body | `{"live":…,"built":…,"revision":…}` on `/livez` and `/healthz` | `curl -s …/healthz` | +| Backend boot log | `revision=…` on the `apiary-backend listening` line | `docker logs … \| grep listening` | +| Frontend static file | `/build.json` → `{"built":…,"revision":…,"source":…}` | `curl -s …/build.json` | +| Both images | `org.opencontainers.image.revision` (with `.source`, `.created`) | `docker image inspect --format '{{ index .Config.Labels "org.opencontainers.image.revision" }}' apiary-backend:latest` | + +The health body and `build.json` are small enough to read unfiltered, so +there is nothing to parse here and nothing that needs `jq` — which is +deliberately absent from these hosts (`verify-deploy.sh` uses `python3` for +the same reason). + +**Where the value comes from, and what `"unknown"` means.** `GIT_SHA` is a +Docker **build arg**, declared in both Dockerfiles and passed by the stacks' +compose files as `build.args.GIT_SHA: ${GIT_SHA:-}`. The backend compiles it +in (`build.rs` re-exports `GIT_SHA` as `APIARY_GIT_SHA`); the frontend writes +it into `public/build.json` before `npm run build`. Unset is normal and not +an error — you get the literal string `unknown`, never a fabricated revision, +because a plausible-looking wrong answer is worse than no answer. Both tiers +normalize to a bare lowercase hex object name (optional `sha256:` prefix +stripped, 7–64 hex chars) or `unknown`, and both are held to one shared table, +`backend-service/src/revision-corpus.json`, so the two normalizers — different +languages, different CI lanes — cannot drift. + +`build.rs` also emits `cargo:rerun-if-env-changed=GIT_SHA`. That line is +load-bearing, not hygiene: cargo caches a build script's output by its inputs +and an environment variable is not one of them unless declared, so without it +the *second* build of an identical tree silently reports the *first* build's +revision — the exact failure this section exists to end, reproduced by the +stamp itself. + +**Turning it on.** Nothing repo-tracked writes `GIT_SHA`; the stacks' `.env` +files are root-owned and provisioned outside this repository, and there is no +`.git` on the host to ask (`deploy.yml` rsyncs with `--exclude .git/`). So +the first run after this landed correctly reports **not stamped**. From a +clone you are deploying, one line per stack: + +```console +echo "GIT_SHA=$(git rev-parse HEAD)" >> /var/dockge/stacks/honeypot-dashboard-backend/.env +echo "GIT_SHA=$(git rev-parse HEAD)" >> /var/dockge/stacks/honeypot-dashboard/.env +``` + +followed by the `POST /projects/{id}/build` that a content-change sync does +*not* do. CI (`.github/workflows/containers.yml`) passes `github.sha` for the +two Dockerfiles that declare the arg. + +**Running the check.** `scripts/verify-deploy.sh` compares the live revision, +the image label, and an expected revision — which it takes from `origin/main` +in a real clone, or from an argument: + +```console +# On a host with a clone of apiary, the full check: +scripts/verify-deploy.sh --healthz-exec \ + 'docker exec hp-apiary-backend curl -sf http://127.0.0.1:8081/healthz' \ + --image apiary-backend:latest + +# Without one, name the expected revision yourself: +scripts/verify-deploy.sh --behind-days 0 "$(git rev-parse origin/main)" \ + --healthz-exec 'docker exec hp-apiary-backend curl -sf http://127.0.0.1:8081/healthz' + +# The frontend's form: +scripts/verify-deploy.sh --healthz-url http://host:19090/build.json +``` + +Exit codes are the point, and are deliberately three-valued: **0** the +deployed revision matches; **1** a finding (missing, unstamped, malformed, or +stale past `--behind-days`); **2** *could not tell* — an unreadable body or an +image that is not on this host. An unreadable answer must never read as a +stale deploy, which is why 2 is not 1. Add `--image` to check a label against +the running container, which catches the commoner variant where a +`compose up` recreated from an older image than the one whose label you just +read. The lag check measures the age of the *commit*, not of the container, so +it survives a container that has been up since before the merge it is missing +— but it needs a clone to measure against, hence `--behind-days 0` for the +clone-less form. + +`diagnostics.yml` runs it in `--warn-only` mode in its own "Deployed +revision" section, against `$GITHUB_SHA`: the home runner has no clone to +measure against, so the section degrades to "is the running revision the +current tip of `main`" and says so in the report. Its exit 2 is still +reported as **could not tell**, not as a pass. ## Disk space monitoring From 51925b8eae3a40d31821a7ef55b2b632bfbc615a Mon Sep 17 00:00:00 2001 From: OpenCode Date: Sun, 27 Sep 2026 16:12:04 +0200 Subject: [PATCH 2/2] test(#3315): give the arkime stash a private path so tests/docs stops reporting the host The "Docs regression tests (#2576)" lane, "Scripts and Compose" and "Quality gate" are one failure, not three. All 20-odd tests in test_3283_fix.py died on the homeserver runner before reaching an assert: composable-templates: reading /tmp/arkime-composable-shadow/shadow.json: EACCES: permission denied run_script passed no SHADOW_FILE, so composable-templates.js fell back to its default -- one fixed name under os.tmpdir(). It opens that path 0700 and the file 0600 for a reason (#3343: the stash is the only record that db.pl's templates existed), so the first user to touch the directory owns it and every later run by another user is locked out. On a fresh container nothing else has been there and the suite is green; on a long-lived runner it is not, and which of those you get depends on the machine rather than the code. The suite's sibling already had this right: test_3343_fix.py passes SHADOW_FILE and inherits os.environ. This brings 3283 in line, one TemporaryDirectory per run, so the env stays the closed set it was rather than becoming os.environ. Two tests pin it, and both fail without the SHADOW_FILE line: - the default path under a private TMPDIR is planted unreadable, which is the runner's exact condition, and the run must still succeed. Planting it in the real /tmp would make the test depend on the shared state it is asserting independence from. - `shadow` then `generate` is how the stash travels in production, so that is the pairing driven here. The second run must not put back a template it never shadowed. generate alone cannot catch this: it reads the stash but never writes it. Not a chmod, and not a skip. The suite must not need to write outside its own tmpdir to run at all. Unrelated and left alone: two scripts/tests failures in test_compose_drift_watch_sweep (privileged-fallback rows) reproduce with this change stashed, and that CI row is green on this branch. Refs #3315 Co-authored-by: OpenCode --- tests/docs/test_3283_fix.py | 148 ++++++++++++++++++++++++++++++++++-- 1 file changed, 143 insertions(+), 5 deletions(-) diff --git a/tests/docs/test_3283_fix.py b/tests/docs/test_3283_fix.py index d7dcf2fd..2882fb4b 100644 --- a/tests/docs/test_3283_fix.py +++ b/tests/docs/test_3283_fix.py @@ -51,6 +51,7 @@ import re import shutil import subprocess +import tempfile import threading import urllib.parse from contextlib import contextmanager @@ -230,16 +231,50 @@ def get(self, key, default=None): return computed if computed is not None else default -def run_script(routes: dict, *, env: dict | None = None, expect_ok: bool = True): - """Run the real script against a stub cluster; return (calls, result).""" - with stub_cluster(routes) as server: +def run_script(routes: dict, *, env: dict | None = None, args: list[str] | None = None, + expect_ok: bool = True): + """Run the real script against a stub cluster; return (calls, result). + + `args` is the subcommand, and the default (no subcommand) is the script's + own `generate`. The two are separate processes in production too -- the + stash is written by `shadow` either side of db.pl and read by `generate` + after it -- so a test that wants to see stash state travel has to drive + both. + + The stash gets a fresh private path per call, and that is load-bearing + rather than tidiness. `SHADOW_FILE` unset means the script falls back to + its default, a *fixed* name under `os.tmpdir()` -- one name shared by + every run of this suite, by test_3343_fix.py, and by any other job + running on the same host. composable-templates.js opens that path 0700 + and the file 0600, so the moment anything else owns the directory first, + every run here dies with + + reading /tmp/arkime-composable-shadow/shadow.json: EACCES + + before it asserts anything. That is not hypothetical: this suite goes + green in a fresh container and red on the homeserver runner, where + /tmp/arkime-composable-shadow is long-lived and shared. Which is also + why the fix is SHADOW_FILE and not a chmod -- a suite must not depend on + the permissions of a machine-wide directory it does not own, and it must + not need to be able to write outside its own tmpdir to be run at all. + + The env stays otherwise minimal on purpose (it is a closed set, not + os.environ), so adding SHADOW_FILE here rather than inheriting the + parent environment keeps that property. + """ + with stub_cluster(routes) as server, tempfile.TemporaryDirectory() as stash: # The handler records into server.calls; the computed-route hook needs # the same object, so it is wired here rather than in __init__. handler_calls = server.calls - environ = {"PATH": "/usr/bin:/bin", "ARKIME__elasticsearch": f"http://127.0.0.1:{server.server_address[1]}"} + shadow_file = pathlib.Path(stash) / "shadow.json" + environ = { + "PATH": "/usr/bin:/bin", + "ARKIME__elasticsearch": f"http://127.0.0.1:{server.server_address[1]}", + "SHADOW_FILE": str(shadow_file), + } environ.update(env or {}) result = subprocess.run( - [NODE, str(SCRIPT)], capture_output=True, text=True, env=environ, timeout=60 + [NODE, str(SCRIPT), *(args or [])], capture_output=True, text=True, env=environ, timeout=60 ) calls = list(handler_calls) if expect_ok: @@ -487,6 +522,109 @@ def test_a_cluster_with_no_sessions_indices_still_succeeds(): assert "0 index(es) adopted, 0 already on" in result.stdout, result.stdout +# -------------------------------------------------------------------------- +# The stash this suite uses +# -------------------------------------------------------------------------- + + +@needs_node +def test_the_stash_is_ours_alone_and_never_the_default_tmp_path(tmp_path): + """A run must not consult composable-templates.js's default stash path. + + The regression this pins: run_script passed no SHADOW_FILE, so the script + fell back to its default -- one fixed name under os.tmpdir(), opened 0700 + with the file 0600. On the homeserver runner that directory already + belongs to another user, and all 20-odd tests in this file died on + `EACCES` before reaching an assert, while the same commit was green in a + clean container. A suite whose pass/fail depends on who owns a shared + directory reports the machine, not the code. + + TMPDIR is pointed at a private directory and that default path is + poisoned, which reproduces the runner's condition exactly -- the script + resolves os.tmpdir() to this dir, computes exactly the path that is + unreadable, and would fail on it if it consulted the default at all. + Reproducing the condition rather than fixing the machine is the point: + planting a decoy in the real /tmp would make this test depend on, and + disturb, the same shared state it is asserting independence from. + """ + hostile_root = tmp_path / "hostile-tmp" + decoy_dir = hostile_root / "arkime-composable-shadow" + decoy_dir.mkdir(parents=True) + decoy = decoy_dir / "shadow.json" + decoy.write_text("{}", encoding="utf-8") + # Unreadable rather than merely foreign-owned, so the check does not + # depend on which uid the suite runs as. The directory stays traversable + # so the failure under test is the file open -- which is the one the + # homeserver runner produced. + decoy.chmod(0o000) + + calls, result = run_script(_default_responses(), env={"TMPDIR": str(hostile_root)}) + + assert result.returncode == 0, ( + "run_script consulted the default stash path under TMPDIR:\n" + f"stdout:\n{result.stdout}\nstderr:\n{result.stderr}" + ) + # And the real work still happened -- isolation is not a way to make the + # script a no-op. + assert body_for(calls, "PUT", f"/_ilm/policy/{POLICY}") is not None, ( + "the run did nothing, so a green result here would prove nothing" + ) + + +@needs_node +def test_two_runs_do_not_see_each_others_stash(tmp_path): + """Each run gets its own stash, so no test can inherit another's state. + + The same fixed-default-path bug as the test above, seen from the other + side: even with nothing else on the host, two runs into one path let the + second see what the first stashed. A suite that depends on run order to be + correct is a suite that breaks when pytest-xdist or a reordering lands. + + The cluster here has a composable template covering an Arkime family, so + the `shadow` pass really does stash and delete it -- without that the + stash stays empty and a leak would be invisible to any assertion. The + two runs are `shadow` then `generate`, which is the pairing production + uses: one stashes, a later process restores. + """ + routes = _default_responses() + routes[("GET", "/_index_template")] = (200, {"index_templates": [ + { + "name": "operator-catch-all", + "index_template": {"index_patterns": ["arkime_sessions3-*"]}, + }, + ]}) + routes[("DELETE", "/_index_template/operator-catch-all")] = (200, {"acknowledged": True}) + shared_root = tmp_path / "shared-tmp" + shared_root.mkdir() + env = {"TMPDIR": str(shared_root)} + + shadowed, shadow_result = run_script(routes, env=env, args=["shadow"]) + generated, generate_result = run_script(routes, env=env, args=["generate"]) + + for result in (shadow_result, generate_result): + assert result.returncode == 0, ( + f"a run failed:\nstdout:\n{result.stdout}\nstderr:\n{result.stderr}" + ) + # The precondition, so a green result below cannot be vacuous: the first + # run must actually have stashed and deleted something. Membership rather + # than body_for: a DELETE carries no body, so a body check would read + # "was called" as "was not called". + def called(calls, method, path): + return (method, path) in [(m, p) for m, p, _ in calls] + + assert called(shadowed, "DELETE", "/_index_template/operator-catch-all"), ( + f"nothing was shadowed, so a leak would be invisible here: {mutations(shadowed)}" + ) + # The second run is the tell. A shared stash means it inherits the + # operator-catch-all body and puts it back -- a write the first run's own + # state does not account for, and the clearest possible statement that + # one run's state reached another's. + assert not called(generated, "PUT", "/_index_template/operator-catch-all"), ( + "the generate pass restored a template this run never shadowed, so it " + f"inherited the previous run's stash: {mutations(generated)}" + ) + + # -------------------------------------------------------------------------- # DRY_RUN # --------------------------------------------------------------------------