ci: harden CI against silent hangs — timeouts, asn1c caching, failures - #61
Merged
Conversation
…rrors install_asn1c_from_source() unconditionally rebuilt asn1c from source on every job and wrapped the whole sequence in `(...) > log 2>&1 || true`, with every individual command inside also guarded by its own `|| true`. On PR #52 this meant a hung dependency install ran silently for six hours (GitHub's hard job ceiling) with zero diagnostic output, and a failed checkout of the pinned commit would have gone unnoticed too, silently building an unpinned tree. Add a short-circuit, keyed on a stamp file recording which commit is currently installed (not just binary presence, so a stale /opt/asn1c from a different pin is rebuilt rather than trusted), so an already-populated /opt/asn1c matching the pinned commit -- e.g. restored from an actions/cache entry, wired up in the next commit -- skips the rebuild entirely. The stamp also protects commit-trailers.yml's per-commit-build job, which checks out several commits in the same container: without it, an earlier commit pinning a different asn1c revision would silently reuse whatever happened to already be on disk. Remove the per-command `|| true` on the steps that must be able to fail the build (checkout, autoreconf, configure, make, make install), and capture the subshell's real exit status via `set +e` / inner `set -e` around it, printing the last 200 lines of the install log before propagating the failure. `sudo ldconfig` is dropped too: confirmed from the pinned fork's Makefile.am that `make install` only ever populates /opt/asn1c/bin and /opt/asn1c/share, never a shared library, so it was a no-op. Verified against isolated fixtures (a throwaway local git repo standing in for the real asn1c upstream, so no network/sudo needed): a failing checkout aborts immediately with the real error surfaced via tail instead of a swallowed `|| true`; a simulated successful build writes the binary and a matching stamp; a matching stamp+binary short-circuits with no rebuild attempted; and a present binary with a mismatched stamp still triggers a full rebuild. Separately, a full Release build + `ctest` (17/17) passes unchanged on this machine's already-installed asn1c. Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Andrea Lacava <a.lacava@northeastern.edu>
…ures Sized from measured passing job durations (GitHub Actions API, successful runs only) with ~2-3x headroom: add timeout-minutes to every job across pr-tests.yml, commit-trailers.yml, full-loop-bench.yml, mpmc-bench.yml, and tag-and-release.yml that had none at all. On PR #52 "Build and Test (ubuntu-latest, Debug)" burned GitHub's full 6-hour job ceiling inside dependency install; the values here would have caught it within minutes. e2e-dapp-test.yml and e2e-topologies.yml already had timeouts on their main jobs, but their "Post PR comment" aggregation jobs did not - fixed too. Cache the from-source asn1c build (actions/cache, keyed on `hashFiles('build_libe3')` plus runner os/arch so the pinned commit bump in the previous commit auto-invalidates every workflow's cache with no YAML edits, and amd64/arm64 never share an entry) instead of switching to the distro package: Ubuntu's asn1c is 0.9.28+dfsg-5 from upstream vlm/asn1c, a different fork and version than the pinned mouse07410/asn1c commit this codebase's 0.9.29-era assumptions rely on. `/opt` is root:root on GitHub-hosted runners, so a `sudo mkdir -p && chown` step has to run before the cache action can create/restore into /opt/asn1c on a fresh VM. In pr-tests.yml, e2e-dapp-test.yml, and e2e-topologies.yml - the three files where several matrix legs would otherwise race to rebuild asn1c in parallel on a cold cache - add an upfront `asn1c-cache` job the matrix jobs `needs:`, so the cache is always warm by the time they start instead of every leg paying the cost independently. Remove the workflow-level `./build_libe3 -I || true` in the 6 places that had one (pr-tests.yml's 4 build jobs, commit-trailers.yml's per-commit-build, mpmc-bench.yml's benchmark), so the previous commit's now-honest script failures actually redden the CI step instead of being masked a second time at the workflow level. Drop the `branches: [main]` filter from `pull_request:` triggers in pr-tests.yml, full-loop-bench.yml, mpmc-bench.yml, e2e-dapp-test.yml, and e2e-topologies.yml. That filter matches the PR's *base* branch, so today's open stack (#51->#52->#53->#58, each based on the previous PR's branch instead of main) gets no build signal at all - only commit-trailers.yml's checks, since that workflow never had the filter. Each file's own cost gate (the `changes` job, `paths-ignore`, or `paths` allowlist) already controls when the expensive jobs run, independent of base branch, so this is a narrow, safe change. Add `concurrency`/`cancel-in-progress` to pr-tests.yml, commit-trailers.yml, and mpmc-bench.yml, matching the pattern already used by full-loop-bench.yml and both E2E workflows, so a superseded push to the same PR doesn't leave a stale run burning CI minutes alongside the new one. `latrec-portability.yml` isn't on main yet (lands with the still-open PR #59) so it isn't touched here; its owner has already added a timeout-minutes to it directly on that branch. This commit is CI-only (.github/workflows/*.yml), which the repo's own "Detect code changes" gate classifies as non-code, so it does not exercise the real build/test jobs on this PR - its mechanics are reasoned from the diff and cross-checked with `python3 -c "import yaml"` for every file plus a job-by-job timeout-minutes audit, not observed passing in a live run the way the previous commit's build_libe3 change was. Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Andrea Lacava <a.lacava@northeastern.edu>
22 tasks
Contributor
⏱️ Full-loop Latency Benchmark (commit
|
| Phase | mean | p50 | p99 | max |
|---|---|---|---|---|
| 1. Collect indication data | 0.20 | 0.18 | 0.36 | 1.72 |
| 2. Create & encode indication | 1.03 | 0.95 | 1.88 | 15.06 |
| 3. Deliver indication (RAN -> dApp) | 111.89 | 107.28 | 190.32 | 220.22 |
| 4. Decode indication | 0.76 | 0.69 | 1.41 | 15.71 |
| 5. Process data | 0.03 | 0.03 | 0.04 | 0.14 |
| 6. Create & encode control | 0.44 | 0.39 | 0.80 | 15.20 |
| 7. Deliver control (dApp -> RAN) | 127.08 | 120.24 | 214.95 | 249.71 |
| 8. Decode & handle control | 0.70 | 0.67 | 1.02 | 16.21 |
| Total round-trip | 242.13 | 208.62 | 351.41 | 393.46 |
Benchmarked on
ubuntu-latest, Release build, ZMQ + IPC, ASN.1 APER.
Contributor
📊 MPMC Queue Benchmark Results (commit
|
| Queue Capacity | P50 (ns) | P95 (ns) | P99 (ns) | P99.9 (ns) |
|---|---|---|---|---|
| 16 | 1137 | 1188 | 1247 | 20213 |
| 64 | 1327 | 3332 | 4264 | 30712 |
| 256 | 1068 | 7590 | 30612 | 31975 |
| 1024 | 36203 | 40982 | 41432 | 41583 |
| 4096 | 102867 | 134727 | 137963 | 138664 |
LockFreeQueue End-to-End Latency (SPSC, 30000 items per queue size)
| Queue Capacity | P50 (ns) | P95 (ns) | P99 (ns) |
|---|---|---|---|
| 16 | 1328 | 2330 | 2471 |
| 64 | 21219 | 25567 | 37029 |
| 256 | 24544 | 32007 | 47867 |
| 1024 | 110293 | 130511 | 139819 |
| 4096 | 468979 | 504386 | 509103 |
Throughput (400000 items per configuration)
| Configuration | Queue Cap | Throughput (Mops/s) |
|---|---|---|
| SPSC (1P×1C) | 256 | 15.57 |
| MPSC (4P×1C) | 1024 | 12.34 |
| MPMC (4P×4C) | 4096 | 11.60 |
| MPMC (8P×4C) | 4096 | 10.49 |
Stress / Correctness (100000 items each)
| Test | Items | Result |
|---|---|---|
| SPSC (1P×1C) | 100000 | ✅ PASS |
| MPSC (4P×1C) | 100000 | ✅ PASS |
| MPMC (4P×4C) | 100000 | ✅ PASS |
| MPMC (8P×8C) | 100000 | ✅ PASS |
Benchmarked on
ubuntu-latest, Release build
Contributor
🔀 E2E Topologies — multi-dApp / multi-RAN (commit
|
Contributor
🔄 E2E dApp Integration Results (commit
|
The branch ruleset on main requires 15 status-check contexts, several of which are gated on "did this PR touch code" -- either a runtime job-level `if:` (pr-tests.yml, commit-trailers.yml) or a trigger-level `paths-ignore` that skips the whole workflow (e2e-dapp-test.yml, e2e-topologies.yml, full-loop-bench.yml). Required-status-checks does not treat a "skipped" conclusion, or a context that never reports at all, as satisfied -- confirmed against this repo's own history: PR #43 ("docs: adopt OAI-style fast-forward merge...") merged with "Each commit builds and tests", "SWIG Python Bindings", "All Encodings", "Integration Tests", and "Build and Test" all showing conclusion=skipped, and several E2E/Topologies/Full-loop-latency contexts never reporting at all. That merge only went through because of the repo's all-or-nothing admin bypass, which is also the thing that lets a merge through without its build actually having been checked -- see the "protect main" ruleset-split conversation this PR enables. Fix that at the source instead: every required-check job now runs unconditionally and reports a real `success`, moving the docs-only skip down to individual steps. - pr-tests.yml: removed the job-level `if:` on asn1c-cache/test, and added `needs: changes` to integration/all-encodings/swig (they only had `needs: test` before, which isn't enough to reference needs.changes.outputs.code); every expensive step in all five jobs is now step-gated instead. - commit-trailers.yml: per-commit-build keeps its job-level `if: github.event_name == 'pull_request'` (an event-type check, unrelated to this fix, and this context is only meaningful for that event anyway) but the code-change check moved to its steps. - e2e-dapp-test.yml, e2e-topologies.yml, full-loop-bench.yml: replaced `paths-ignore` with an unconditional trigger plus the same runtime `changes` job pattern already used in pr-tests.yml (robust to workflow_dispatch, which has no PR base/head to diff against). full-loop-bench.yml's "Post results as PR comment" step reads bench_results.md unconditionally with no graceful missing-file handling (unlike the E2E jobs' upload-artifact steps, which already have `if-no-files-found: warn`), so it needed its own gate to avoid crashing the job outright on a docs-only PR. Also gated the (already non-required) `comment` jobs in both E2E files the same way, so they don't spend a run trying to aggregate artifacts that were never produced. Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Andrea Lacava <a.lacava@northeastern.edu>
Observed live on this PR's own CI, twice in a row and in two different call sites: a plain `sudo apt-get update` hung silently for 12-18 minutes against azure.archive.ubuntu.com (Ign:/retry lines, then total silence) before the job's own timeout-minutes finally killed it. One occurrence was in pr-tests.yml's asn1c-cache job, inside build_libe3's own install_dependencies(); the other was in e2e-topologies.yml's topologies job, in the workflow-level `sudo apt-get update` step that ran before calling `./build_libe3 -I` at all. Same mirror, same symptom, two different commands -- the timeout-minutes added earlier in this PR caught both correctly (that's the point), but a job dying after 15-20 minutes on a transient mirror hiccup is still a bad outcome worth avoiding when the hiccup usually clears on retry. Add retry_apt(): wraps a package-manager command in `timeout 120`, retrying up to 3 times. 120s comfortably covers an actually-successful `apt-get update` (observed ~45s) or the `install` package list, while still failing fast enough that 3 attempts fit well inside the existing job timeouts. Wire it into install_dependencies()'s apt-get branch (both the `update` and `install` calls). Also remove the workflow-level `sudo apt-get update` step that preceded `./build_libe3 -I` in every job across all 7 workflow files (~15 call sites): it was calling apt-get update a second time only seconds before install_dependencies() calls it again internally -- pure redundant work, and one more independent place the same mirror flakiness could (and did) strike. The SWIG job's separate `sudo apt-get install -y swig python3-dev` is similarly redundant (swig and python3-dev are already in install_dependencies()'s own package list) and was removed too, along with its now-orphaned update call. Verified retry_apt() directly: succeeds immediately on a working command; exhausts all attempts and returns non-zero on a command that always fails; and, with a shortened timeout substituted in for a fast test, confirmed a hanging command is killed at the timeout boundary on every attempt rather than blocking indefinitely. Full local Release build + `ctest` (17/17) still passes. Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Andrea Lacava <a.lacava@northeastern.edu>
Pushed the previous commit's flat 120s retry_apt budget and watched it fail for real, twice, on this PR's own CI: every attempt of `apt-get install` hit the full 120s wall while still visibly downloading real packages (up through an 11MB cmake package) rather than sitting idle -- a mirror that's genuinely slow right now, not hung, and 120s isn't enough headroom for a full ~18-package install under that condition. `apt-get update` (metadata only) has consistently succeeded within 90s across every run observed so far, including this session's own slower ones (45-89s). Give retry_apt() an explicit per-attempt timeout parameter instead of a hardcoded one, and size each call site from what was actually observed: 90s for `apt-get update`, 180s for the `apt-get install` package list. Also bump the four job timeouts that were at exactly 15 minutes and call install_dependencies (asn1c-cache in pr-tests.yml/ e2e-dapp-test.yml/e2e-topologies.yml, Build and Test, E2E, and Full-loop latency) to 20, so 3 retries at the new, more realistic per-attempt budgets can't crowd out the job's own real work under today's observed mirror conditions. Full local Release build + `ctest` (17/17) still passes. Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Andrea Lacava <a.lacava@northeastern.edu>
Thecave3
changed the title
Pushed the previous commit's timing fix and watched a different, sharper failure on this PR's own CI: E2E posix/ipc's install step hit "E: Could not get lock /var/lib/dpkg/lock. It is held by process 2750 (dpkg)" on attempts 2 and 3, both firing within milliseconds of attempt 1 rather than after their own 180s budget. `timeout 180 sudo apt-get install ...` sends its kill signal to the direct child, which is `sudo`, not `apt-get`; sudo does not reliably forward that signal on to its own child. So when attempt 1's wall-clock ran out mid-install (observed at "Reading database ... 90%"), dpkg kept running orphaned underneath the killed sudo/apt-get, still holding the lock -- and every retry failed instantly against that stale lock instead of getting a fresh timeout budget at all. Reorder to `sudo timeout N apt-get ...`: timeout is now the direct parent of apt-get, so its signal reaches the right process. Also run `dpkg --configure -a` between attempts (best-effort) so a dpkg run that still gets interrupted has a chance to finish or clean up before the next retry. Verified with a stubbed sudo (this dev machine has no passwordless sudo, so a real one can't be exercised locally): success returns immediately; a command that always fails exhausts all 3 attempts in milliseconds (each one genuinely failing fast, not waiting out its budget); a hanging command is killed at each attempt's timeout boundary. Full local Release build + `ctest` (17/17) still passes. Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Andrea Lacava <a.lacava@northeastern.edu>
"Post fast-forward merge command" only ran on an approved
pull_request_review, which never fires for a solo-authored PR --
GitHub doesn't allow self-approval and there's no second reviewer, so
this context sat permanently "skipped". Harmless to mergeability
(it isn't itself a required check), but it also meant the one job
meant to hand the maintainer the exact merge command never actually
ran for the PRs in this repo's current stack.
Decouple it from review state entirely: what actually makes the
comment trustworthy is that the required checks for this exact commit
are green, not who clicked approve. The job now polls the branch's
*effective* required status checks (via `gh api
repos/{repo}/rules/branches/main`, so it can't drift out of sync with
whatever the ruleset currently requires -- no hardcoded context list
or ruleset ID) until they've all completed, then posts the comment
only if every one succeeded. Bumped its timeout-minutes from 5 to 20
to fit the poll (up to 18 minutes, generous for a same-repo PR's own
checks) plus the post itself.
Verified the script directly with a stubbed `gh`, since a real
end-to-end run needs live checks in progress: waits through pending
polls before posting; declines to post when a required check failed;
updates an existing ready-to-merge comment instead of duplicating it;
and gives up quietly past its deadline without posting. Will confirm
it fires for real on this PR's own next push, now that PR #61's
checks are already green.
Assisted-by: Claude:claude-sonnet-5
Signed-off-by: Andrea Lacava <a.lacava@northeastern.edu>
Contributor
Ready to merge (fast-forward only)All required checks are green for git fetch origin
git checkout main && git merge --ff-only 3b09e95dcb68c0bd1e9617c38248727f829b5964 && git push origin mainHead: |
Thecave3
added a commit
that referenced
this pull request
Aug 19, 2026
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-minutesto every job that lacked one acrosspr-tests.yml,commit-trailers.yml,full-loop-bench.yml,mpmc-bench.yml, andtag-and-release.yml, sized from measured passing durations (GitHub Actions API, successful runs only) with ~2-3x headroom — the actual gap behind PR fix(encoder): splice JSON indication protocolData instead of parse+dump round trip #52's job burning GitHub's 6-hour hard ceiling silently inside "Install build dependencies".actions/cache, keyed onhashFiles('build_libe3')plus runner os/arch) instead of rebuilding it in ~13 job instances every run, and makebuild_libe3'sinstall_asn1c_from_source()cache-aware and stop swallowing real failures behind|| true.pull_requestbranch filters (they matched the PR's base, so today's stacked PRs fix(queue): notify-on-push wakeup for LockFreeQueue instead of sleep-poll #51→fix(encoder): splice JSON indication protocolData instead of parse+dump round trip #52→fix(examples): make FixedRate pacing wait interruptible so stop() returns promptly #53→fix(encoder): carry Pdu::timestamp on the ASN.1 wire, and make the grammar extensible #58 got no build signal at all) and add missingconcurrency/cancel-in-progressguards.Type of change
Linked issue
Refs #60 (partial). Deliberately not "Closes #60": this issue covers more than this PR alone resolves — #62 fixes the actual E2E/Topologies deadlock, and
latrec-portability.yml's missing timeout is being added directly on PR #59's branch by its author. 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 clean./build/test_bench_mpmc_queue) shows no regression vsmain— ran locally (correctness stress tests all pass); neither commit touchesinclude/libe3/mpmc_queue.hppso no regression is possibleVERSIONbumped per SemVer — N/A, no public API/ABI change./build_libe3 --docs— N/A, noinclude/changes./build_libe3 -I— N/A, no new dependencies added;install_asn1c_from_source()'s error-handling/caching behavior changed, not its dependency listlibe3.pcinterface — N/A, unchangedCI checklist
Unit Testsworkflow is green — will confirm once this PR's own run completes (see "Observed vs. reasoned" below)Commit policyworkflow is green — will confirm once this PR's own run completesMPMC Queue Benchmark— this PR does touchmpmc-bench.ymlitself (adding a timeout/cache step), so itspaths:allowlist means it will run live on this PR; will confirm it stays greenTwin-repo coordination
Paired PR(s): none needed — CI-only change.
Workflow confirmation
main(rebased ifmainmoved), with no merge commits.git bisect-safe) with a descriptive message.CONTRIBUTING.md.Observed vs. reasoned
build_libe3commit (1st commit) — verified against isolated fixtures (a throwaway local git repo standing in for the real asn1c upstream, no network/sudo needed): a failing checkout now aborts immediately with the real error surfaced viatailinstead of a swallowed|| true; a simulated successful build writes the binary and a matching stamp; a matching stamp+binary short-circuits with zero rebuild attempted; a present binary with a mismatched stamp still triggers a full rebuild (protectscommit-trailers.yml's per-commit-build job, which checks out several commits in the same container). Also ran a full local Release and Debug build +ctest(17/17 both) with asn1c already installed on the dev machine, exercising the short-circuit path, plus the MPMC benchmark's correctness suite..github/workflows/*.ymlas non-code, so it doesn't exercise the real build/test jobs on its own. Its mechanics (timeout values, theneeds:-sequencedasn1c-cachewarm-up jobs in the three matrix-heavy files, the cache key/permissions handling, the branch-filter/concurrency changes) are reasoned from the diff and cross-checked with a YAML parser plus a job-by-job timeout-minutes audit script, not yet watched in a live run. Will report back once this PR's own CI has actually run.