Skip to content

Commit 00b8f63

Browse files
rafaelgilnRafaelclaude
authored
fix(migration): give each job its own credential tracker (#1295 follow-up, #793 collision) (#1325)
Follow-up to #1324, which routed a blocked run to a `provider-credentials` tracker so a drained provider account would stop being filed as a Langflow migration failure. That tracker was **shared by both jobs**, which made it a fresh instance of the collision PR #793 found on run #101: the two jobs of `migration-test.yml` run in parallel, and the one that goes green closes whatever open issue carries the label — including one the other job filed a minute earlier on a failure that is still live. Reaching the provider from the compose job says nothing about what the API job saw, and vice versa. - The label is now per job (`provider-credentials-api` / `provider-credentials-compose`, both self-provisioned on first use), composed in one place by `issue_label(job)`. - `--job` is REQUIRED on the CLI with `choices=("api", "compose")`: a default would hand both jobs one tracker, and a typo would open a third nobody watches. - Each `Close issue on success` step closes only its own job's credential label. The `migration-test` half keeps its current shared behaviour — that is #793's scope, not this change's. The cost is that a drained account files two issues, one per job. That is noise which self-clears on the next good run, where the alternative silently closes a live blocker. Validation: 69 tests in the Python unit lane (3 new), including a structural guard that pins an ABSENCE — neither job's half of the workflow may reference the other's credential label or pass the other's `--job` — because these two jobs are near-copies and copy-paste between them is exactly how a per-job label degrades back into a shared one. All 4 mutations attempted were killed: a shared label, an unvalidated job name, the compose job closing the API job's tracker, and the compose pre-flight claiming to be the API job. Probed live against the still-drained key: both jobs exit 1 with their own label. Co-authored-by: Rafael <rafael@oriontech.me> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 453b6e1 commit 00b8f63

6 files changed

Lines changed: 141 additions & 29 deletions

File tree

.github/workflows/migration-test.yml

Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ jobs:
7171
OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }}
7272
run: |
7373
python tests/github-workflows/migration/provider_credentials.py \
74-
--probe --phase "pre-flight/pip"
74+
--probe --phase "pre-flight/pip" --job api
7575
7676
- name: Install uv
7777
uses: astral-sh/setup-uv@v7
@@ -324,9 +324,14 @@ jobs:
324324
// old version said "Migration test passed" on whatever open
325325
// `migration-test` issue it found, which is how a billing outage filed
326326
// there would have been closed as a fixed migration bug.
327+
//
328+
// The credential label is THIS job's own (`-api`), never the compose job's:
329+
// the two run in parallel, and closing the other one's issue is the
330+
// collision PR #793 found on run #101. The `migration-test` half keeps its
331+
// existing (shared) behaviour — that is #793's scope, not this PR's.
327332
const CLOSING = {
328333
'migration-test': 'Migration test passed. Closing this issue.',
329-
'provider-credentials':
334+
'provider-credentials-api':
330335
'This run reached the model provider and executed the witness flow, so the ' +
331336
'credential that blocked it is usable again. Closing.',
332337
};
@@ -405,7 +410,7 @@ jobs:
405410
repo: context.repo.repo,
406411
name: label,
407412
color: 'd93f0b',
408-
description: 'Provider credential/billing state blocked a scheduled run (no product signal)',
413+
description: `Provider credential/billing state blocked the migration test's ${credential.job} job (no product signal)`,
409414
});
410415
} catch (e) {
411416
console.log(`Label ${label} already exists or could not be created:`, e.message);
@@ -492,7 +497,7 @@ jobs:
492497
OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }}
493498
run: |
494499
python3 tests/github-workflows/migration/provider_credentials.py \
495-
--probe --phase "pre-flight/compose"
500+
--probe --phase "pre-flight/compose" --job compose
496501
497502
- name: Generate ephemeral SECRET_KEY
498503
run: |
@@ -687,7 +692,7 @@ jobs:
687692
# deliberately does not change the exit code below.
688693
python3 tests/github-workflows/migration/provider_credentials.py \
689694
--classify-file /tmp/run-source.json --status "$HTTP" \
690-
--phase "compose/source-execute" || true
695+
--phase "compose/source-execute" --job compose || true
691696
exit 1
692697
fi
693698
echo "Source flow execution OK"
@@ -763,7 +768,7 @@ jobs:
763768
# here would have filed exactly that against Langflow (#1295).
764769
python3 tests/github-workflows/migration/provider_credentials.py \
765770
--classify-file /tmp/run-target.json --status "$HTTP" \
766-
--phase "compose/target-execute" || true
771+
--phase "compose/target-execute" --job compose || true
767772
exit 1
768773
fi
769774
if grep -iE 'api[ _-]?key.{0,30}(required|missing|not[ _]?found)|fernet|cannot[ _]decrypt' /tmp/run-target.json; then
@@ -849,10 +854,11 @@ jobs:
849854
uses: actions/github-script@v9
850855
with:
851856
script: |
852-
// Two trackers, same reasoning as the API job above (#1295).
857+
// Two trackers, same reasoning as the API job above (#1295) — and the
858+
// credential label is this job's own (`-compose`), for the #793 reason.
853859
const CLOSING = {
854860
'migration-test': 'docker-compose migration test passed. Closing.',
855-
'provider-credentials':
861+
'provider-credentials-compose':
856862
'This run reached the model provider and executed the witness flow, so the ' +
857863
'credential that blocked it is usable again. Closing.',
858864
};
@@ -914,7 +920,7 @@ jobs:
914920
repo: context.repo.repo,
915921
name: label,
916922
color: 'd93f0b',
917-
description: 'Provider credential/billing state blocked a scheduled run (no product signal)',
923+
description: `Provider credential/billing state blocked the migration test's ${credential.job} job (no product signal)`,
918924
});
919925
} catch (e) {
920926
console.log(`Label ${label} already exists or could not be created:`, e.message);

tests/github-workflows/migration/README.md

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,9 @@ On failure, the workflow opens or updates an issue in the repository, including
5050
| Verdict | Tracker | Meaning |
5151
|---|---|---|
5252
| `FAILED` | `migration-test` | Something about the migration is broken — a claim about Langflow. |
53-
| `BLOCKED (provider credentials)` | `provider-credentials` | The witness flow never reached the model provider, so **nothing about the migration was measured**. Not a claim about Langflow; fix the account or the key. |
53+
| `BLOCKED (provider credentials)` | `provider-credentials-api` / `provider-credentials-compose` | The witness flow never reached the model provider, so **nothing about the migration was measured**. Not a claim about Langflow; fix the account or the key. |
54+
55+
The credential tracker is **per job**, and each job closes only its own. The two jobs run in parallel, so a shared label lets the one that goes green close the issue the other just filed — the collision PR #793 found on run #101, where the API job's pass closed the compose job's live failure. The cost is that a drained account files two issues; they self-clear on the next good run, whereas the alternative silently closes a live blocker.
5456

5557
The split exists because both are red and only one is about the product. A billing outage filed under `migration-test` gets closed by the next green run with *"Migration test passed"*, leaving a migration bug in the history that never existed. A green run closes **both** trackers, each with wording about what it actually proved.
5658

tests/github-workflows/migration/provider_credentials.py

Lines changed: 41 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -139,7 +139,26 @@
139139
# The label the report is routed to when the verdict is blocking. Kept off
140140
# `migration-test` so a later green run cannot close it with "Migration test
141141
# passed", which would read as a migration bug that never existed.
142-
ISSUE_LABEL = "provider-credentials"
142+
#
143+
# **Per job, not shared** — the collision PR #793 found on run #101: the two jobs of
144+
# this workflow run in parallel and used one label, so the job that went green closed
145+
# the issue the other had just filed (its `migration-test` failure survived as
146+
# "resolved"). A single `provider-credentials` label would have been a fresh instance
147+
# of that same bug: the compose job reaching the provider says nothing about what the
148+
# API job saw a minute earlier, and vice versa. The cost is that a drained account
149+
# files two issues — one per job — which is noise, but noise that self-clears on the
150+
# next good run, where the alternative silently closes a live blocker.
151+
ISSUE_LABEL_BASE = "provider-credentials"
152+
153+
# The two jobs of `migration-test.yml`, spelled as they appear in `--job`. A closed
154+
# set so a typo in the workflow cannot quietly open a third tracker nobody watches.
155+
JOBS = ("api", "compose")
156+
157+
158+
def issue_label(job: str) -> str:
159+
if job not in JOBS:
160+
raise ValueError(f"unknown job {job!r} — expected one of {JOBS}")
161+
return f"{ISSUE_LABEL_BASE}-{job}"
143162

144163

145164
def classify(status: Optional[int], body: str) -> Tuple[str, str]:
@@ -239,18 +258,23 @@ def _send(request: urllib.request.Request, timeout: int) -> Tuple[int, str]:
239258
return err.code, err.read().decode("utf-8", "replace")
240259

241260

242-
def marker_payload(verdict: str, reason: str, phase: str) -> dict:
261+
def marker_payload(verdict: str, reason: str, phase: str, job: str) -> dict:
243262
return {
244263
"verdict": verdict,
245264
"reason": reason,
246265
"phase": phase,
266+
"job": job,
247267
"title": _TITLES.get(verdict, ""),
248268
"action": _ACTIONS.get(verdict, ""),
249-
"label": ISSUE_LABEL,
269+
# The workflow's issue step reads this rather than composing it, so the
270+
# per-job split lives in one place.
271+
"label": issue_label(job),
250272
}
251273

252274

253-
def write_marker(verdict: str, reason: str, phase: str, path: str = MARKER_FILE) -> Optional[str]:
275+
def write_marker(
276+
verdict: str, reason: str, phase: str, job: str, path: str = MARKER_FILE
277+
) -> Optional[str]:
254278
"""Record a blocking verdict for the workflow's issue-routing step.
255279
256280
Only `BLOCKING` verdicts are written: the file's **presence** is the signal, and
@@ -260,11 +284,13 @@ def write_marker(verdict: str, reason: str, phase: str, path: str = MARKER_FILE)
260284
if verdict not in BLOCKING:
261285
return None
262286
with open(path, "w") as handle:
263-
json.dump(marker_payload(verdict, reason, phase), handle, indent=2)
287+
json.dump(marker_payload(verdict, reason, phase, job), handle, indent=2)
264288
return path
265289

266290

267-
def step_status(detail: str, phase: str, marker: Optional[str] = None) -> str:
291+
def step_status(
292+
detail: str, phase: str, job: str = "api", marker: Optional[str] = None
293+
) -> str:
268294
"""The status a failed flow execution should record: `"blocked"` or `"fail"`.
269295
270296
The mid-run half of #1295, shared by `setup_latest.py` and
@@ -273,12 +299,15 @@ def step_status(detail: str, phase: str, marker: Optional[str] = None) -> str:
273299
string, where the provider's own error is quoted inside Langflow's — hence no
274300
status to pass. A blocking verdict is announced and recorded on the way out; any
275301
other failure keeps its original `"fail"` and this function stays silent.
302+
303+
`job` defaults to `"api"` because both callers are steps of that job — the compose
304+
job has no Python and classifies through the CLI.
276305
"""
277306
verdict, reason = classify(None, detail)
278307
if verdict not in BLOCKING:
279308
return "fail"
280309
print(announce(verdict, reason, phase))
281-
write_marker(verdict, reason, phase, MARKER_FILE if marker is None else marker)
310+
write_marker(verdict, reason, phase, job, MARKER_FILE if marker is None else marker)
282311
return "blocked"
283312

284313

@@ -310,6 +339,10 @@ def main(argv=None) -> int:
310339
parser.add_argument("--status", type=int, default=None, help="HTTP status for --classify-file")
311340
parser.add_argument("--model", default=DEFAULT_MODEL)
312341
parser.add_argument("--phase", default="pre-flight", help="where the verdict was reached")
342+
# Required, with no default: which job filed this decides which issue a green run
343+
# is allowed to close, and a default would hand both jobs the same tracker — the
344+
# #793 collision (see ISSUE_LABEL_BASE).
345+
parser.add_argument("--job", required=True, choices=JOBS, help="which job reached the verdict")
313346
parser.add_argument("--marker", default=MARKER_FILE)
314347
args = parser.parse_args(argv)
315348

@@ -329,7 +362,7 @@ def main(argv=None) -> int:
329362
verdict, reason = classify(args.status, body)
330363

331364
print(announce(verdict, reason, args.phase))
332-
written = write_marker(verdict, reason, args.phase, args.marker)
365+
written = write_marker(verdict, reason, args.phase, args.job, args.marker)
333366
if written:
334367
print(f"Recorded credential verdict in {written}")
335368

tests/github-workflows/migration/setup_latest.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,7 @@ def main():
112112
# the common case before this job installs anything; this covers the account
113113
# draining mid-run, and records `blocked` — a state the report renders apart from
114114
# FAILED, and the workflow routes to the credentials tracker, not `migration-test`.
115-
status = "pass" if success else credentials.step_status(detail, "latest/execute_flow")
115+
status = "pass" if success else credentials.step_status(detail, "latest/execute_flow", job="api")
116116
print(f" {status.upper()}: {detail[:120]}")
117117
phase["steps"]["execute_flow"] = {"status": status, "detail": detail}
118118

tests/github-workflows/migration/test_provider_credentials.py

Lines changed: 78 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -258,21 +258,50 @@ def send(request, timeout):
258258

259259
def test_marker_is_written_only_for_blocking_verdicts(tmp_path):
260260
path = tmp_path / "verdict.json"
261-
assert pc.write_marker(pc.LIVE, "fine", "pre-flight", str(path)) is None
262-
assert pc.write_marker(pc.INCONCLUSIVE, "unclear", "pre-flight", str(path)) is None
261+
assert pc.write_marker(pc.LIVE, "fine", "pre-flight", "api", str(path)) is None
262+
assert pc.write_marker(pc.INCONCLUSIVE, "unclear", "pre-flight", "api", str(path)) is None
263263
assert not path.exists(), "absence of the marker is what keeps the default routing"
264264

265-
assert pc.write_marker(pc.BILLING, "drained", "pre-flight", str(path)) == str(path)
265+
assert pc.write_marker(pc.BILLING, "drained", "pre-flight", "api", str(path)) == str(path)
266266
payload = json.loads(path.read_text())
267267
assert payload["verdict"] == pc.BILLING
268-
assert payload["label"] == pc.ISSUE_LABEL
268+
assert payload["label"] == "provider-credentials-api"
269269
assert payload["label"] != "migration-test", (
270270
"routing a billing state to the migration tracker is #1295 — a later green "
271271
"run would close it with 'Migration test passed'"
272272
)
273+
assert payload["job"] == "api"
273274
assert payload["title"] and payload["action"]
274275

275276

277+
def test_the_two_jobs_get_two_different_trackers(tmp_path):
278+
"""The #793 collision, which a single shared label would have reintroduced.
279+
280+
Both jobs run in parallel; the one that goes green must not be able to close the
281+
issue the other just filed, because reaching the provider from the compose job
282+
says nothing about what the API job saw a minute earlier.
283+
"""
284+
labels = {job: pc.issue_label(job) for job in pc.JOBS}
285+
286+
assert labels["api"] != labels["compose"]
287+
assert set(labels.values()) == {"provider-credentials-api", "provider-credentials-compose"}
288+
assert "migration-test" not in labels.values()
289+
290+
for job, label in labels.items():
291+
path = tmp_path / f"{job}.json"
292+
pc.write_marker(pc.BILLING, "drained", f"pre-flight/{job}", job, str(path))
293+
assert json.loads(path.read_text())["label"] == label
294+
295+
296+
def test_an_unknown_job_is_refused_rather_than_given_a_tracker():
297+
# A typo in the workflow must not open a third tracker nobody watches.
298+
import pytest
299+
300+
for job in ("", "API", "pip", "compose-2", None):
301+
with pytest.raises(ValueError):
302+
pc.issue_label(job)
303+
304+
276305
def test_a_blocking_announcement_states_there_was_no_migration_signal():
277306
line = pc.announce(pc.BILLING, "drained", "pre-flight")
278307
assert line.startswith("::error::")
@@ -316,6 +345,39 @@ def test_both_api_scripts_route_their_execution_failure_through_step_status():
316345
)
317346

318347

348+
def test_neither_job_can_touch_the_other_jobs_credential_tracker():
349+
"""Structural, and it pins an ABSENCE — which is the realistic regression here.
350+
351+
The two jobs of `migration-test.yml` are near-copies of each other, so the way a
352+
per-job label degrades back into a shared one is a copy-paste between them (that
353+
is how the collision PR #793 found reached both halves in the first place). Text,
354+
not YAML parsing: the PR-validation lane installs `pytest` and nothing else, so
355+
`pyyaml` is not available here.
356+
357+
It cannot tell whether the routing is *correct* — the tests above do that.
358+
"""
359+
import pathlib
360+
361+
workflow = (
362+
pathlib.Path(__file__).resolve().parents[3] / ".github/workflows/migration-test.yml"
363+
).read_text()
364+
365+
marker = "\n migration-test-compose:"
366+
assert marker in workflow, "the compose job header moved — this guard cannot split the file"
367+
api_half, compose_half = workflow.split(marker, 1)
368+
369+
for half, own, other in (
370+
(api_half, "api", "compose"),
371+
(compose_half, "compose", "api"),
372+
):
373+
assert f"provider-credentials-{other}" not in half, (
374+
f"the {own} job references the {other} job's credential tracker — a green run "
375+
f"would close an issue about a failure it never observed (#793)"
376+
)
377+
assert f"--job {own}" in half, f"the {own} job does not tell the classifier which job it is"
378+
assert f"--job {other}" not in half
379+
380+
319381
def test_any_other_failure_still_records_fail_and_says_nothing(tmp_path, capsys):
320382
marker = tmp_path / "verdict.json"
321383

@@ -340,7 +402,7 @@ def test_probe_mode_exits_nonzero_on_billing(monkeypatch, tmp_path, capsys):
340402
monkeypatch.setattr(pc, "_send", _transport(429, RUN_124_BODY))
341403
marker = tmp_path / "verdict.json"
342404

343-
code = pc.main(["--probe", "--marker", str(marker)])
405+
code = pc.main(["--probe", "--job", "api", "--marker", str(marker)])
344406

345407
assert code == 1, "a blocking pre-flight must stop the job before it installs anything"
346408
assert "::error::" in capsys.readouterr().out
@@ -351,7 +413,7 @@ def test_probe_mode_exits_zero_when_inconclusive(monkeypatch, tmp_path, capsys):
351413
monkeypatch.setenv("OPENAI_API_KEY", "sk-test")
352414
monkeypatch.setattr(pc, "_send", _transport(503, "bad gateway"))
353415

354-
code = pc.main(["--probe", "--marker", str(tmp_path / "verdict.json")])
416+
code = pc.main(["--probe", "--job", "api", "--marker", str(tmp_path / "verdict.json")])
355417

356418
assert code == 0, "fail-open: the run itself is the authoritative verdict"
357419
assert "::warning::" in capsys.readouterr().out
@@ -370,6 +432,8 @@ def test_classify_file_mode_attributes_without_changing_the_exit_code(tmp_path,
370432
"500",
371433
"--phase",
372434
"compose-source",
435+
"--job",
436+
"compose",
373437
"--marker",
374438
str(marker),
375439
]
@@ -383,7 +447,7 @@ def test_classify_file_mode_attributes_without_changing_the_exit_code(tmp_path,
383447

384448

385449
def test_an_unreadable_body_file_never_masks_the_real_failure(tmp_path, capsys):
386-
code = pc.main(["--classify-file", str(tmp_path / "absent.json")])
450+
code = pc.main(["--classify-file", str(tmp_path / "absent.json"), "--job", "compose"])
387451
assert code == 0
388452
out = capsys.readouterr().out
389453
assert "::warning::" in out and "no credential verdict" in out
@@ -393,6 +457,11 @@ def test_probe_and_classify_file_are_mutually_exclusive(tmp_path):
393457
import pytest
394458

395459
with pytest.raises(SystemExit):
396-
pc.main(["--probe", "--classify-file", str(tmp_path / "x.json")])
460+
pc.main(["--probe", "--job", "api", "--classify-file", str(tmp_path / "x.json")])
461+
with pytest.raises(SystemExit):
462+
pc.main(["--job", "api"])
463+
# `--job` itself is required: a default would hand both jobs one tracker (#793).
464+
with pytest.raises(SystemExit):
465+
pc.main(["--probe"])
397466
with pytest.raises(SystemExit):
398-
pc.main([])
467+
pc.main(["--probe", "--job", "pip"])

tests/github-workflows/migration/verify_migration_api.py

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,9 @@ def main():
8585
# migration-specific checks above (flow preserved, variables preserved) still ran
8686
# and still fail on their own terms if they are broken.
8787
status = (
88-
"pass" if success else credentials.step_status(detail, "nightly_api/execute_flow_api")
88+
"pass"
89+
if success
90+
else credentials.step_status(detail, "nightly_api/execute_flow_api", job="api")
8991
)
9092
print(f" {status.upper()}: {detail[:120]}")
9193
phase["steps"]["execute_flow_api"] = {"status": status, "detail": detail}

0 commit comments

Comments
 (0)