Skip to content

Commit 3986d17

Browse files
authored
[CRCR] Fix multi-job state key collision by adding job_name to Redis key (#8228)
## Summary - PR #8170 changed the Redis state key from `check_run_id` (unique per job) to `run_id:run_attempt` (unique per workflow run), causing state machine collisions when multiple jobs in the same workflow run sent callbacks concurrently - This fix preserves PR #8170's re-run correlation intent while adding per-job uniqueness via `job_name` (`github.job`, already present in callback payloads) - The state key is now `crcr:state:{delivery_id}:{repo}:{run_id}:{run_attempt}:{job_name}`, with `job_name` omitted for DISPATCHED records (backward-compatible) ## Test plan - [ ] All 78 unit tests pass locally (verified) - [ ] Trigger 3 successful runs on `TorchedHat/pytorch-redhat-ci` L2 edge case workflow (multi-job `ec05-a`/`ec05-b` jobs no longer collide) - [ ] Verify single-job workflows still work (no regression from optional `job_name`)
1 parent a5e8cc4 commit 3986d17

6 files changed

Lines changed: 245 additions & 79 deletions

File tree

aws/lambda/cross_repo_ci_relay/callback/callback_handler.py

Lines changed: 19 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -60,10 +60,11 @@ def _verify_access(
6060
return allowlist, repo_level
6161

6262

63-
def _parse_callback_body(body: dict) -> tuple[str, str, int, int, str]:
64-
"""Return (delivery_id, status, run_id, run_attempt, workflow_name) from ``body``.
63+
def _parse_callback_body(body: dict) -> tuple[str, str, int, int, str, str | None]:
64+
"""Return (delivery_id, status, run_id, run_attempt, workflow_name, job_name).
6565
66-
run_id and run_attempt uniquely identify a workflow run execution.
66+
run_id and run_attempt identify a workflow run execution.
67+
job_name (``github.job``) disambiguates multiple jobs within the same run.
6768
run_attempt defaults to 1 when not present in the callback body.
6869
6970
Raises HTTPException(400) on any missing or mis-typed field.
@@ -72,17 +73,16 @@ def _parse_callback_body(body: dict) -> tuple[str, str, int, int, str]:
7273
delivery_id = body["delivery_id"]
7374
workflow_dict = body["workflow"]
7475
status = workflow_dict["status"]
75-
run_id = int(workflow_dict["run_id"]) # Required for HUD grouping
76-
run_attempt = int(
77-
workflow_dict.get("run_attempt", 1)
78-
) # Default to 1 if not present
79-
workflow_name = workflow_dict["name"] # Required for HUD grouping
76+
run_id = int(workflow_dict["run_id"])
77+
run_attempt = int(workflow_dict.get("run_attempt", 1))
78+
workflow_name = workflow_dict["name"]
79+
job_name = workflow_dict.get("job_name")
8080
except (KeyError, TypeError) as exc:
8181
logger.warning(f"missing required field in callback body: {exc}")
8282
raise HTTPException(
8383
400, f"callback body missing required field: {exc}"
8484
) from exc
85-
return delivery_id, status, run_id, run_attempt, workflow_name
85+
return delivery_id, status, run_id, run_attempt, workflow_name, job_name
8686

8787

8888
def _update_state_and_compute_metrics(
@@ -96,6 +96,7 @@ def _update_state_and_compute_metrics(
9696
dispatch_record: CallbackStateRecord,
9797
workflow_record: CallbackStateRecord | None,
9898
payload: dict | None = None,
99+
job_name: str | None = None,
99100
) -> dict:
100101
"""Persist the new workflow state to Redis and return CI timing metrics.
101102
@@ -129,6 +130,7 @@ def _update_state_and_compute_metrics(
129130
current_timestamp,
130131
workflow_name,
131132
payload=payload,
133+
job_name=job_name,
132134
)
133135
except RedisError:
134136
raise HTTPException(
@@ -144,7 +146,7 @@ def _update_state_and_compute_metrics(
144146
raise
145147

146148
updated_workflow_record = redis_helper.get_callback_state(
147-
config, delivery_id, verified_repo, run_id, run_attempt
149+
config, delivery_id, verified_repo, run_id, run_attempt, job_name=job_name
148150
)
149151
if updated_workflow_record is None:
150152
return ci_metrics
@@ -188,7 +190,9 @@ def handle(config: RelayConfig, body: dict, verified_repo: str) -> dict:
188190
return {"ok": True, "status": "ignored"}
189191
_, repo_level = result
190192

191-
delivery_id, status, run_id, run_attempt, workflow_name = _parse_callback_body(body)
193+
delivery_id, status, run_id, run_attempt, workflow_name, job_name = (
194+
_parse_callback_body(body)
195+
)
192196

193197
dispatch_record = redis_helper.get_callback_state(
194198
config, delivery_id, verified_repo, DISPATCH_RUN_ID, DISPATCH_RUN_ATTEMPT
@@ -202,7 +206,7 @@ def handle(config: RelayConfig, body: dict, verified_repo: str) -> dict:
202206
raise HTTPException(400, "callback rejected: no matching dispatch record")
203207

204208
workflow_record = redis_helper.get_callback_state(
205-
config, delivery_id, verified_repo, run_id, run_attempt
209+
config, delivery_id, verified_repo, run_id, run_attempt, job_name=job_name
206210
)
207211

208212
payload = None
@@ -230,16 +234,16 @@ def handle(config: RelayConfig, body: dict, verified_repo: str) -> dict:
230234
dispatch_record,
231235
workflow_record,
232236
payload=payload,
237+
job_name=job_name,
233238
)
234239

235-
# Track in-progress jobs for zombie detection.
236240
if status == "in_progress":
237241
redis_helper.add_in_progress_tracker(
238-
config, delivery_id, verified_repo, run_id, run_attempt
242+
config, delivery_id, verified_repo, run_id, run_attempt, job_name=job_name
239243
)
240244
elif status == "completed":
241245
redis_helper.remove_in_progress_tracker(
242-
config, delivery_id, verified_repo, run_id, run_attempt
246+
config, delivery_id, verified_repo, run_id, run_attempt, job_name=job_name
243247
)
244248

245249
trusted = {

aws/lambda/cross_repo_ci_relay/callback/cleanup_handler.py

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -68,29 +68,30 @@ def _cleanup_one(
6868
repo = zombie["downstream_repo"]
6969
run_id = zombie["run_id"]
7070
run_attempt = zombie["run_attempt"]
71+
job_name = zombie.get("job_name")
7172
hud_ok = True
7273

73-
# 1. Build and forward timeout payload to HUD
7474
try:
7575
trusted, untrusted = _build_timeout_payload(zombie, completed_at)
7676
forward_to_hud(config, trusted, untrusted)
7777
logger.info(
78-
"zombie HUD forward succeeded repo=%s run_id=%s run_attempt=%s",
78+
"zombie HUD forward succeeded repo=%s run_id=%s run_attempt=%s job_name=%s",
7979
repo,
8080
run_id,
8181
run_attempt,
82+
job_name,
8283
)
8384
except Exception:
8485
logger.exception(
85-
"zombie HUD forward failed repo=%s run_id=%s run_attempt=%s",
86+
"zombie HUD forward failed repo=%s run_id=%s run_attempt=%s job_name=%s",
8687
repo,
8788
run_id,
8889
run_attempt,
90+
job_name,
8991
)
9092
hud_ok = False
9193

9294
if hud_ok:
93-
# 2. Mark state as COMPLETED in Redis (best-effort)
9495
try:
9596
redis_helper.set_callback_state(
9697
config,
@@ -100,22 +101,20 @@ def _cleanup_one(
100101
run_attempt,
101102
CallbackState.COMPLETED,
102103
time.time(),
104+
job_name=job_name,
103105
)
104106
except (AssertionError, RedisError):
105-
# Race: another process already resolved this record, or Redis
106-
# was unavailable.
107107
logger.warning(
108108
"zombie state transition failed (may already be resolved) "
109-
"delivery_id=%s repo=%s run_id=%s run_attempt=%s",
109+
"delivery_id=%s repo=%s run_id=%s run_attempt=%s job_name=%s",
110110
delivery_id,
111111
repo,
112112
run_id,
113113
run_attempt,
114+
job_name,
114115
)
115-
# Erase the records from Redis only when HUD was successfully
116-
# updated, keeping the two systems in sync.
117116
redis_helper.remove_in_progress_tracker(
118-
config, delivery_id, repo, run_id, run_attempt
117+
config, delivery_id, repo, run_id, run_attempt, job_name=job_name
119118
)
120119

121120
return {"ok": hud_ok}

aws/lambda/cross_repo_ci_relay/tests/test_callback_handler.py

Lines changed: 50 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -19,23 +19,32 @@ def _cfg():
1919
return cfg
2020

2121

22-
def _body(status="completed", workflow_name="default", run_id=99999, run_attempt=1):
22+
def _body(
23+
status="completed",
24+
workflow_name="default",
25+
run_id=99999,
26+
run_attempt=1,
27+
job_name=None,
28+
):
29+
wf = {
30+
"status": status,
31+
"conclusion": "success" if status == "completed" else None,
32+
"name": "CI",
33+
"url": "http://ci.example.com/run/1",
34+
"workflow_name": workflow_name,
35+
"run_id": run_id,
36+
"run_attempt": run_attempt,
37+
}
38+
if job_name is not None:
39+
wf["job_name"] = job_name
2340
return {
2441
"event_type": "pull_request",
2542
"delivery_id": "del-123",
2643
"payload": {
2744
"pull_request": {"number": 42, "head": {"sha": "abc123"}},
2845
"repository": {"full_name": "pytorch/pytorch"},
2946
},
30-
"workflow": {
31-
"status": status,
32-
"conclusion": "success" if status == "completed" else None,
33-
"name": "CI",
34-
"url": "http://ci.example.com/run/1",
35-
"workflow_name": workflow_name,
36-
"run_id": run_id,
37-
"run_attempt": run_attempt,
38-
},
47+
"workflow": wf,
3948
}
4049

4150

@@ -54,7 +63,13 @@ def setUp(self):
5463

5564
# Setup default: dispatch exists, workflow state is None (in_progress not yet reported)
5665
def default_get_state(
57-
cfg, delivery_id, repo, run_id_arg, run_attempt_arg, client=None
66+
cfg,
67+
delivery_id,
68+
repo,
69+
run_id_arg,
70+
run_attempt_arg,
71+
client=None,
72+
job_name=None,
5873
):
5974
if run_id_arg == DISPATCH_RUN_ID:
6075
return CallbackStateRecord(
@@ -234,6 +249,30 @@ def test_redis_error_fetching_workflow_record_proceeds(self):
234249
_, trusted_arg, _ = self.mock_hud.call_args[0]
235250
self.assertIsNone(trusted_arg["ci_metrics"]["execution_time"])
236251

252+
def test_job_name_passed_to_redis_state_calls(self):
253+
"""job_name from callback body is forwarded to all Redis state calls."""
254+
dispatch_record = CallbackStateRecord(CallbackState.DISPATCHED, 1000.0, {})
255+
in_progress_record = CallbackStateRecord(CallbackState.IN_PROGRESS, 1030.0, {})
256+
self.mock_redis.get_callback_state.side_effect = [
257+
dispatch_record,
258+
None,
259+
in_progress_record,
260+
]
261+
262+
handle(
263+
_cfg(),
264+
_body(status="in_progress", job_name="build"),
265+
verified_repo="org/repo",
266+
)
267+
268+
# set_callback_state should have been called with job_name="build"
269+
call_kwargs = self.mock_redis.set_callback_state.call_args
270+
self.assertEqual(call_kwargs.kwargs.get("job_name"), "build")
271+
272+
# add_in_progress_tracker should have been called with job_name="build"
273+
tracker_kwargs = self.mock_redis.add_in_progress_tracker.call_args
274+
self.assertEqual(tracker_kwargs.kwargs.get("job_name"), "build")
275+
237276

238277
if __name__ == "__main__":
239278
unittest.main()

aws/lambda/cross_repo_ci_relay/tests/test_cleanup_handler.py

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,7 @@ def _zombie_entry(
5454
repo="org/repo",
5555
run_id=99999,
5656
run_attempt=1,
57+
job_name="my-job",
5758
in_progress_ts=None,
5859
body_overrides=None,
5960
):
@@ -79,6 +80,7 @@ def _zombie_entry(
7980
"downstream_repo": repo,
8081
"run_id": run_id,
8182
"run_attempt": run_attempt,
83+
"job_name": job_name,
8284
"state_record": CallbackStateRecord(
8385
CallbackState.IN_PROGRESS, in_progress_ts, stored_payload
8486
),
@@ -186,22 +188,21 @@ def test_cleans_single_zombie(self):
186188
untrusted["callback_payload"]["workflow"]["conclusion"], "timed_out"
187189
)
188190

189-
# Redis state was updated to COMPLETED
191+
# Redis state was updated to COMPLETED with job_name
190192
self.mock_redis.set_callback_state.assert_called_once()
191193
call_args = self.mock_redis.set_callback_state.call_args[0]
192194
self.assertEqual(call_args[1], "del-123")
193195
self.assertEqual(call_args[2], "org/repo")
194196
self.assertEqual(call_args[3], 99999)
195197
self.assertEqual(call_args[4], 1)
196198
self.assertEqual(call_args[5], CallbackState.COMPLETED)
199+
call_kwargs = self.mock_redis.set_callback_state.call_args[1]
200+
self.assertEqual(call_kwargs.get("job_name"), "my-job")
197201

198-
# ZSET entry was removed
202+
# ZSET entry was removed with job_name
199203
self.mock_redis.remove_in_progress_tracker.assert_called_once()
200-
rm_args = self.mock_redis.remove_in_progress_tracker.call_args[0]
201-
self.assertEqual(rm_args[1], "del-123")
202-
self.assertEqual(rm_args[2], "org/repo")
203-
self.assertEqual(rm_args[3], 99999)
204-
self.assertEqual(rm_args[4], 1)
204+
rm_kwargs = self.mock_redis.remove_in_progress_tracker.call_args[1]
205+
self.assertEqual(rm_kwargs.get("job_name"), "my-job")
205206

206207
def test_cleans_multiple_zombies(self):
207208
"""Multiple zombies are all cleaned independently."""

0 commit comments

Comments
 (0)