ci(e2e): bound backgrounded agent waits with diagnostics-on-timeout - #62
Merged
Conversation
22 tasks
Contributor
⏱️ Full-loop Latency Benchmark (commit
|
| Phase | mean | p50 | p99 | max |
|---|---|---|---|---|
| 1. Collect indication data | 0.18 | 0.17 | 0.32 | 0.43 |
| 2. Create & encode indication | 1.07 | 1.00 | 1.78 | 13.35 |
| 3. Deliver indication (RAN -> dApp) | 110.98 | 103.09 | 198.65 | 523.40 |
| 4. Decode indication | 0.78 | 0.73 | 1.25 | 15.13 |
| 5. Process data | 0.03 | 0.03 | 0.04 | 0.17 |
| 6. Create & encode control | 0.43 | 0.38 | 0.71 | 18.46 |
| 7. Deliver control (dApp -> RAN) | 120.91 | 111.92 | 205.19 | 449.30 |
| 8. Decode & handle control | 0.66 | 0.64 | 1.02 | 14.80 |
| Total round-trip | 235.04 | 201.30 | 359.54 | 939.04 |
Benchmarked on
ubuntu-latest, Release build, ZMQ + IPC, ASN.1 APER.
Contributor
🔀 E2E Topologies — multi-dApp / multi-RAN (commit
|
Contributor
🔄 E2E dApp Integration Results (commit
|
The dApp side of each agent/dApp pairing in e2e-dapp-test.yml and e2e-topologies.yml is already wrapped in `timeout`, but the backgrounded agent process is only ever sent `kill -INT` and then `wait`ed with no bound at all. If it ignores the signal or is genuinely deadlocked, that wait blocks until the job-level timeout - this is the actual mechanism behind issue #60's intermittent E2E/Topologies hangs, distinct from and complementary to the CI-hardening work in the separate PR against #60 (timeouts, asn1c caching, honest dependency-install failures). This does not root-cause why a peer occasionally gets stuck - it makes the harness itself resilient to that hang: add scripts/wait_with_diagnostics.sh, a bounded replacement for `wait <pid>` that dumps process state (ps stat/wchan, /proc/<pid>/status, open fds) and SIGKILLs the process if it hasn't exited within a given timeout, instead of hanging indefinitely. Wire it into the 5 unbounded agent waits (e2e-dapp-test.yml's single wait; e2e-topologies.yml's three: topology 1 and 2's shared-RAN wait, topology 3's two per-RAN waits). Escalate the existing dApp-side `timeout` calls (5 call sites total) to also SIGKILL after a 10s grace period via `-k 10`, so a dApp that ignores SIGTERM doesn't reopen the same hole on the other side of the pairing. The script lives under scripts/, not .github/, deliberately: both E2E workflows' `paths-ignore` includes `.github/**`, so a PR whose only changed files lived there would never even trigger them, defeating this PR's own ability to self-verify live. These are cleanup/teardown waits, not pass/fail signals - test verdicts already come from the dApp's exit code and indication counts - so a watchdog-triggered kill doesn't flip any topology's own pass/fail logic, it only guarantees the job can't hang on this path. Once this lands and a real hang recurs in CI, the captured diagnostics are the first real evidence for root-causing it - worth a follow-up issue then, with actual data, not now. Verified wait_with_diagnostics() directly against real backgrounded processes, under `set -e` matching the workflows' own shell state: a quick-exiting process returns its real exit status without printing diagnostics; a process that outlives the timeout gets its diagnostics dumped and is SIGKILLed at (measured) the exact timeout boundary. Caught and fixed a real bug in the process - `((waited++))` is a post-increment that evaluates to 0 on the loop's first pass, which bash's `set -e` treats as a failing bare command and would have aborted the whole calling script after one second in any caller that doesn't append `|| true` (every real call site here does, but the function needed to be correct on its own); switched to `((++waited))` and confirmed via a bare, unguarded call that the full timeout window now elapses correctly. Both workflow files still parse as valid YAML; the live E2E/Topologies runs on this PR (which touches the new script, so paths-ignore won't skip them) are the actual test of the happy path end to end. Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Andrea Lacava <a.lacava@northeastern.edu>
Thecave3
force-pushed
the
60-e2e-watchdog-timeout
branch
from
August 19, 2026 23:05
d317377 to
cb08be5
Compare
Contributor
Ready to merge (fast-forward only)All required checks are green for git fetch origin
git checkout main && git merge --ff-only 7398166996483b222712b92b67552ef4e8629ff7 && git push origin mainHead: |
Discovered live on this branch's own rebased CI run, right after merging #61: the "ready to merge" comment posted while "Build and Test (ubuntu-latest, Release)" was still in_progress. The polling loop iterated over whatever check-runs the API happened to return and only flipped `all_done` to false for entries it found with status != completed -- a required context that hasn't been *created* yet at all (a matrix leg still queued behind another job) never appears in that response, so the loop silently never touched it and `all_done` stayed true. Doesn't affect actual merge safety: the branch ruleset independently re-checks every required context at push time regardless of what this comment says, so a premature comment couldn't have caused an unsafe merge, only a premature or misleading informational one. Still worth fixing since it undermines the point of gating the comment on real completion at all, and it was already merged into main via #61. Fix: iterate over the required list, not over the found check-runs, and look up each one's current status individually. A required context missing from the response now correctly counts as "not done" instead of being silently skipped. Verified with a stubbed gh covering the exact bug scenario (a required context absent from two consecutive polls, appearing only on the third) plus the four scenarios from the original commit (waits then posts, declines on a real failure, updates an existing comment, gives up quietly past its deadline) -- all five now correct. Full local Release build + `ctest` (17/17) still passes. Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Andrea Lacava <a.lacava@northeastern.edu>
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
timeout, but the backgrounded agent process is only ever sentkill -INTand thenwaited unbounded — if it ignores the signal or is genuinely deadlocked, that wait blocks until the 15/20-minute job timeout.scripts/wait_with_diagnostics.sh: a boundedwaitreplacement that dumps process state (psstat/wchan,/proc/<pid>/status, open fds) and SIGKILLs a stuck peer instead of hanging, wired into all 5 unbounded agent waits acrosse2e-dapp-test.yml/e2e-topologies.yml.timeoutcalls (5 sites) to also SIGKILL after a grace period (-k 10), closing the same hole on the other side of each pairing.Type of change
Linked issue
Refs #60 (the actual deadlock fix). Deliberately not "Closes #60" — same reasoning as #61, which covers the rest of #60's CI-hardening bullets. Closing #60 is left to a maintainer once everything lands.
Mandatory test checklist
./build_libe3 -c -d build -j $(nproc) -r -tpasses (Release build + tests) — verified locally, 17/17 tests passed./build_libe3 -c -d build -j $(nproc) -g -tpasses (Debug build + tests) — verified locally, 17/17 tests passedcd build && ctest --output-on-failureis cleanmain— ran locally; this PR doesn't touchinclude/libe3/mpmc_queue.hppVERSIONbumped — N/A, no public API/ABI change./build_libe3 --docs— N/A, noinclude/changeslibe3.pcinterface — N/A, unchangedCI checklist
Unit Testsworkflow is green — this PR doesn't change anythingpr-tests.ymlbuilds/tests differently; will confirm it stays greenCommit policyworkflow is green — will confirm once this PR's own run completesMPMC Queue Benchmark— N/A, doesn't run on this diff (mpmc_queue.hppuntouched)Twin-repo coordination
Paired PR(s): none needed — CI test-harness change only.
Workflow confirmation
main(rebased ifmainmoved), with no merge commits.git bisect-safe) with a descriptive message.CONTRIBUTING.md.Observed vs. reasoned
wait_with_diagnostics()tested directly against real backgrounded processes underset -e(matching the workflows' actual shell state) — a quick-exiting process returns cleanly with no diagnostics printed; a process that outlives its timeout gets diagnostics dumped and is SIGKILLed at the measured timeout boundary (verified withdate-based timing, not just visual inspection). Also ran the actuale2e-dapp-test.ymlagent/dApp pairing logic locally end-to-end against the realexample_simple_agent/example_simple_dappbinaries (zmq/ipc): the dApp received all 7 indications and exited 0, the agent stopped cleanly onSIGINT, andwait_with_diagnosticsreaped it without needing to fire its kill path.((waited++))is a post-increment that evaluates to 0 on the loop's first pass, which bash'sset -etreats as a failing bare command — would have aborted the calling script after one second in any caller lacking a trailing|| true(every real call site here has one, but the function needed to be correct standalone). Fixed via((++waited))and reverified with a bare, unguarded call.paths-ignoredoesn't skip them) will at least confirm the happy path across all matrix legs; will report back once watched.