V0.3.1 wip

.claude/skills/delete-releases/SKILL.md

@@ -0,0 +1,33 @@
+---
+name: delete-releases
+description: Delete all GitHub releases for a given E2SAR version
+disable-model-invocation: true
+user-invocable: true
+argument-hint: <version>
+allowed-tools: Bash
+---
+
+Delete all GitHub releases matching version `$ARGUMENTS`.
+
+Releases follow the naming pattern: `E2SAR-<version>-<branch>-<os>-<distro_version>` where os is ubuntu, rocky, or debian.
+
+Steps:
+
+1. First list all releases matching the version to show the user what will be deleted:
+   ```
+   gh release list --limit 100 | grep "E2SAR-$ARGUMENTS-"
+   ```
+
+2. Show the list to the user and ask for confirmation before deleting.
+
+3. If the user confirms, delete each matching release:
+   ```
+   gh release list --limit 100 | grep "E2SAR-$ARGUMENTS-" | awk '{print $1}' | xargs -I {} gh release delete {} --yes 
+   ```
+
+4. Verify deletion by listing releases again to confirm they are gone.
+
+Important:
+- Always show the user what will be deleted BEFORE deleting
+- Never clean up the associated tags
+- If no releases match, inform the user and do nothing

.claude/skills/review-pr/SKILL.md

@@ -0,0 +1,221 @@
+---
+name: review-pr
+description: Perform a structured code review of an E2SAR GitHub PR and post inline + summary comments
+user-invocable: true
+argument-hint: <PR-number>
+allowed-tools: Bash, Read, Glob, Grep
+---
+
+Perform a thorough, structured code review of E2SAR pull request `$ARGUMENTS` and post actionable GitHub comments.
+
+## Steps
+
+### 1. Fetch PR metadata and diff
+
+```bash
+gh pr view $ARGUMENTS --json number,title,body,author,baseRefName,headRefName
+gh repo view --json nameWithOwner --jq '.nameWithOwner'   # capture as REPO
+gh api repos/REPO/pulls/$ARGUMENTS/files --jq '.[].filename'
+```
+
+Display a summary to the user: PR title, author, base branch, files changed.
+
+To read the actual changed lines for a file, use the per-file patch from the API — **do not parse `gh pr diff` with awk**, as range patterns silently drop content when the next `diff --git` header is adjacent:
+
+```bash
+# All patches at once (good for small PRs)
+gh api repos/REPO/pulls/$ARGUMENTS/files --jq '.[] | {path: .filename, patch: .patch}'
+
+# One file at a time
+gh api repos/REPO/pulls/$ARGUMENTS/files \
+  --jq '.[] | select(.filename == "path/to/file.cpp") | .patch'
+```
+
+### 2. Categorize changed files
+
+Group the changed files by type and apply the matching checklist in Step 3:
+
+| Pattern | Checklist |
+|---------|-----------|
+| `include/*.hpp`, `src/*.cpp` | C++ style + docs |
+| `src/pybind/py_*.cpp` | pybind11 bindings |
+| `test/py_test/test_*.py` | pytest conventions |
+| `test/*.cpp` | Boost.Test conventions |
+| `meson.build` | Build system |
+| `RELEASE-NOTES.md`, `VERSION.txt` | Version hygiene |
+
+For context on project conventions, read the relevant reference files as needed:
+- `include/e2sarError.hpp` — `E2SARErrorc` enum and `E2SARErrorInfo`
+- `include/e2sarURI.hpp` — naming/docs conventions
+- `include/e2sarDPSegmenter.hpp` — Flags struct, thread state, atomics
+- `include/e2sarCP.hpp` — gRPC `result<T>` pattern
+- `src/pybind/py_e2sarDP.cpp` — pybind11 patterns
+- `test/py_test/test_b2b_DP.py` — pytest helper/marker conventions
+- `test/py_test/pytest.ini` — official list of valid pytest markers
+
+### 3. Apply review checklists
+
+#### C++ checklist (`include/*.hpp`, `src/*.cpp`)
+
+**Naming conventions**
+- Classes: PascalCase (`Segmenter`, `LBManager`)
+- Methods: camelCase (`openAndStart()`, `sendEvent()`); private helpers prefixed with `_` (`_open()`, `_threadBody()`)
+- Getters: `get_` prefix; setters: `set_` prefix; boolean queries: `has_` / `is` prefix
+- Variables: camelCase; constants/constexpr: UPPER_SNAKE_CASE
+- Type aliases: PascalCase with `_t` suffix (`EventNum_t`, `UnixTimeNano_t`)
+
+**Documentation**
+- All public class methods in headers must have a Doxygen `/** ... */` block with `@param` per parameter and `@return`
+- Constructors must document every parameter
+- `noexcept` methods must be declared as such in the signature
+- Deleted copy constructors/assignment operators must have a brief `/** Don't want to copy... */` comment
+- Structs with multiple fields should have a descriptive block listing each field
+
+**Error handling**
+- Constructors throw `E2SARException` on failure — must not silently swallow errors
+- Methods returning `result<T>` must be marked `noexcept`
+- Callers of `result<T>` must check `has_error()` before `.value()`; direct `.value()` without check is a bug
+- New error codes must be added to `E2SARErrorc` enum in `include/e2sarError.hpp`; never return raw `-1`
+
+**Memory and thread safety**
+- Shared mutable state accessed from multiple threads must use `std::atomic<>` or a mutex
+- New exclusively-owned class members should use `std::unique_ptr`; shared ownership uses `std::shared_ptr`
+- Classes with thread ownership must delete copy constructor and copy assignment
+- Large objects passed into threads should use `std::move`; never pass by copy if avoidable
+- Raw `new` without a corresponding `delete` or RAII wrapper is a memory leak; use `std::unique_ptr<T>` or pool allocator
+
+**Header structure**
+- Header guards: `#ifndef E2SAR<NAME>HPP / #define E2SAR<NAME>HPP`
+- Include order: standard library → third-party (Boost, gRPC) → project headers
+- `using namespace` declarations inside `namespace e2sar {}` only; never at global scope in headers
+- `"string"s` literals require `using namespace std::string_literals;` in scope
+
+#### pybind11 checklist (`src/pybind/py_*.cpp`)
+
+- Each bound method must have a short docstring as the last string argument to `.def()`
+- Python-side names use snake_case (`get_instance_token`, `set_instance_token`)
+- Enum values use lowercase (`.value("admin", ...)`)
+- Any binding that calls a Python callable from a C++ thread must use GIL management (`py::gil_scoped_acquire`)
+- Buffer/numpy methods must use `py::buffer_info` and cast through `buf_info.ptr`
+
+#### pytest checklist (`test/py_test/test_*.py`)
+
+- Every test function must start with a `"""Test <thing being tested>."""` docstring
+- Each test must be decorated with exactly one marker from `pytest.ini`: `@pytest.mark.unit`, `@pytest.mark.b2b`, `@pytest.mark.cp`, or `@pytest.mark.lb-b2b`
+- `result<T>` return values must be checked: `assert res.has_error() is False, f"{res.error().message}"`
+- b2b tests using ports must account for TIME_WAIT; use different port numbers across tests or add socket options
+- Helper functions (not tests) must not have a `test_` prefix
+
+#### Boost.Test checklist (`test/*.cpp`)
+
+- Each test case registered with `BOOST_AUTO_TEST_CASE` should test one logical thing
+- Failures must use `BOOST_CHECK_*` / `BOOST_REQUIRE_*` macros, not raw `assert`
+- New test executables must be registered in `test/meson.build` with `suite: 'unit'` or `suite: 'live'`
+
+#### Meson build checklist (`meson.build`)
+
+- New source files must be listed in the appropriate `meson.build` `sources:` array (silently excluded otherwise)
+- New test executables must be registered with `test('Name', exe, suite: 'unit|live')`
+- New dependencies must follow `dependency('name', version: '>=x.y.z')` pattern
+- Optional features (liburing, NUMA) must use `compiler.has_header()` / `compiler.compiles()` guards
+- `link_args: linker_flags` must be included for all executables/libraries that link e2sar
+
+#### Version hygiene (`RELEASE-NOTES.md`, `VERSION.txt`)
+
+- `VERSION.txt` must be updated if the PR introduces a new release
+- `RELEASE-NOTES.md` entry must match the version in `VERSION.txt`
+- Alpha/beta suffixes (`a1`, `b1`) must be removed before a final release commit
+
+#### Secrets and usernames
+
+- Files must not contain secrets that look real - random strings should be flagged as potential secrets.
+- Strings that look like user names should also be flagged. 
+- Pay special attention to strings that look like 'ejfats://token@host:port/...' or 'ejfat://token@host:port/...' or mention EJFAT_URI. It is allowed to use the word 'token' or 'randomstring' or some variation of this to denote the secret token, however strings that look random likely represent real secrets and must be flagged. 
+
+### 4. Post inline GitHub review comments
+
+For each issue found, post an inline **brief** comment using the GitHub API. Determine the commit SHA and file path from the diff.
+
+First, get the latest commit SHA on the PR head:
+```bash
+gh pr view $ARGUMENTS --json headRefOid --jq '.headRefOid'
+```
+
+Then post each inline comment:
+```bash
+gh api repos/{owner}/{repo} --method GET --jq '.full_name'
+# Use the returned full_name as REPO below
+
+gh api repos/REPO/pulls/$ARGUMENTS/comments \
+  --method POST \
+  --field body='COMMENT_TEXT' \
+  --field commit_id='HEAD_SHA' \
+  --field path='FILE_PATH' \
+  --field line=LINE_NUMBER \
+  --field side='RIGHT'
+```
+
+Get `REPO` once with:
+```bash
+gh repo view --json nameWithOwner --jq '.nameWithOwner'
+```
+
+Use this comment templates (adapt the specific details):
+
+**C++ naming (private helper):**
+> `parseFromString` should be `_parseFromString` — private helpers use the `_` prefix. See `Segmenter::_open()` for the convention.
+
+**Missing Doxygen:**
+> Public method `computeChecksum()` is missing a `/** ... */` Doxygen block. Add `@param` for each argument and `@return` describing the result.
+
+**result<T> misuse:**
+> `res.value()` is called without checking `res.has_error()` first. This will throw if the call fails. Wrap in `if (!res.has_error()) { ... }` or assert.
+
+**noexcept missing:**
+> This method returns `result<T>` but isn't marked `noexcept`. All `result<T>`-returning methods should be `noexcept` — exceptions are encoded in the result type.
+
+**Thread safety:**
+> This counter is incremented from multiple threads without synchronization. Use `std::atomic<uint64_t>` as done in `Segmenter::AtomicStats::errCnt`.
+
+**Missing test marker:**
+> This test function has no pytest marker. Add `@pytest.mark.unit` (or the appropriate category) so it's included in the right test suite.
+
+**Binding docstring missing:**
+> `.def("methodName", ...)` is missing a docstring. Add a brief description as the last string argument.
+
+**Build file omission:**
+> `new_module.cpp` is not listed in `src/meson.build`'s source list. It will be silently excluded from the build.
+
+**Error code missing:**
+> This failure path returns a raw value instead of `E2SARErrorInfo{E2SARErrorc::SocketError, "..."}`. Use the proper error type.
+
+**Memory leak risk:**
+> Raw `new` without a corresponding `delete` or RAII wrapper. Use `std::unique_ptr<T>` or the existing pool allocator.
+
+### 5. Post a summary comment
+
+After all inline comments, post one top-level PR comment with the overall assessment.
+Use a heredoc to avoid quoting failures when the body contains single quotes or backticks:
+
+```bash
+gh pr comment $ARGUMENTS --body "$(cat <<'EOF'
+SUMMARY
+EOF
+)"
+```
+
+The summary must include:
+- **Overall verdict**: Approve / Request Changes / Comment
+- **Blocking issues** (must fix before merge): numbered list, **briefly** describing each problem. 
+- **Non-blocking suggestions** (style/docs): bulleted list **briefly** describing each suggestion
+- **What looks good**: **brief** callouts of well-done sections
+- Footer: `> Review generated by /review-pr skill — verify all inline comments before merging.`
+
+## Important notes
+
+- Only report issues that are actually present in the diff — do not flag pre-existing code outside the changed lines
+- Distinguish blocking issues (correctness, memory safety, build breakage) from non-blocking ones (style, docs)
+- If no issues are found in a category, state that explicitly in the summary rather than inventing feedback
+- Always fetch the repo name dynamically with `gh repo view` — never hardcode it
+- If `$ARGUMENTS` is empty or not a valid PR number, ask the user for the PR number before proceeding
+- Be **brief**, only provide enough information to explain the problem or suggestion. Do not be conversational, instead use **brief** and to-the-point suggestions only.

.github/workflows/pr-review.yml

@@ -0,0 +1,39 @@
+name: Automated PR Review
+
+on:
+  pull_request:
+    types: [opened]   # fires exactly once — when the PR is first opened
+  workflow_dispatch:
+    inputs:
+      prnum_in:
+        description: 'PR Number'
+        type: string
+        required: true
+
+jobs:
+  review:
+    runs-on: ubuntu-latest
+    permissions:
+      contents: read
+      pull-requests: write  # needed so Claude can post inline + summary comments
+      id-token: write
+    steps:
+      - name: Resolve PR number                                                                                                                    
+        id: resolve_pr                                                                                                                             
+        run: |
+          if [ "${{ github.event_name }}" = "workflow_dispatch" ]; then                                                                            
+            echo "PR_NUMBER=${{ inputs.prnum_in }}" >> $GITHUB_ENV                                                                             
+          else                                                                                                                                     
+            echo "PR_NUMBER=${{ github.event.pull_request.number }}" >> $GITHUB_ENV
+          fi
+
+      - name: Checkout code
+        uses: actions/checkout@v4
+
+      - name: Run /review-pr skill
+        uses: anthropics/claude-code-action@beta
+        with:
+          anthropic_api_key: ${{ secrets.ANTHROPIC_API_KEY }}
+          direct_prompt: "/review-pr ${{ env.PR_NUMBER }}"
+        env:
+          GH_TOKEN: ${{ secrets.CLASSIC_REPO_PAT }}  # authenticates gh CLI calls inside the skill

Dockerfile.cli

@@ -5,7 +5,7 @@
 # This is a multi-stage build optimized for minimal image size
 
 # Global build arguments (available to all stages)
-ARG DEPS_VER=0.3.0a1
+ARG DEPS_VER=0.3.1
 ARG E2SAR_DEPS_DEB=E2SAR-${DEPS_VER}-main-ubuntu-24.04/e2sar-deps_${DEPS_VER}_amd64.deb
 ARG E2SAR_DEPS_DEB_URL=https://github.com/JeffersonLab/E2SAR/releases/download/${E2SAR_DEPS_DEB}
 ARG E2SARINSTALL=/e2sar-install

Doxyfile

@@ -48,7 +48,7 @@ PROJECT_NAME           = "E2SAR"
 # could be handy for archiving the generated documentation or if some version
 # control system is used.
 
-PROJECT_NUMBER         = 0.2.0
+PROJECT_NUMBER         = 0.3.0
 
 # Using the PROJECT_BRIEF tag one can provide an optional one line description
 # for a project that appears at the top of each page and should give viewer a

README.md

@@ -267,6 +267,8 @@ E2SAR code comes with a set of tests under [test/](test/) folder. It relies on B
 
 There is a  [Jupyter notebook](scripts/notebooks/EJFAT/LBCP-tester.ipynb) which runs all the tests on FABRIC testbed.
 
+For checking for memory leaks use [scripts/perf-valgrind-loopback.sh](scripts/perf-valgrind-loopback.sh) (run it from scripts/ and give parameters `./perf-valgrind-loopback.sh build /tmp/valgrind-report/` where build is the meson build directory and /tmp/valgrind-report is the directory where the script will place all the logs). Then you can run the XML report parser to get a more concise report `scripts/parse-valgrind-xml.py /tmp/valgrind-reports`. 
+
 ### Python
 
 The code can be tested using pytest

RELEASE-NOTES.md

@@ -2,6 +2,13 @@
 
 API Details can always be found in the [wiki](https://github.com/JeffersonLab/E2SAR/wiki) and in the [Doxygen site](https://jeffersonlab.github.io/E2SAR-doc/annotated.html). 
 
+## v0.3.1
+
+- Reversed the order of removing senders and shutting down send threads in e2sar_perf to ensure no losses occur due to premature de-listing of the sender IP in LB
+- Fixed multiple memory leaks in segmenter and reassembler
+- Fixed proper handling of `-v/--novalidate` flag in lbadm
+- Added 'Zero-to-Hero' scripts that show how to do performance testing on Perlmutter (located in scripts/zero_to_hero). The scripts exercise the wide variety of E2SAR tools and show how they can be integrated into Slurm workflows.
+
 ## v0.3.0
 
 - Dependencies change - Boost 1.89.0 and gRPC 1.74.1 

VERSION.txt

@@ -1 +1 @@
-0.3.0
+0.3.1

bin/e2sar_perf.cpp

@@ -46,6 +46,9 @@ void shutDown()
     boost::chrono::milliseconds duration(1000);
     boost::this_thread::sleep_for(duration);
     if (segPtr != nullptr) {
+        // d-tor will stop the threads - it is important to do that before removing sender
+        // to make sure outstanding data is sent out
+        delete segPtr;
         if (lbmPtr != nullptr) {
             std::cout << "Removing senders: ";
             if (senders.size() > 0)
@@ -64,8 +67,6 @@ void shutDown()
                     std::cerr << "Unable to remove auto-detected sender from list on exit: " << rmres.error().message() << std::endl;
             }
         }
-        // d-tor will stop the threads
-        delete segPtr;
     }
     if (reasPtr != nullptr)
     {

bin/lbadm.cpp

@@ -935,7 +935,7 @@ int main(int argc, char **argv)
         preferV6 = true;
     }
 
-    novalidate = not vm["novalidate"].as<bool>();
+    novalidate = vm["novalidate"].as<bool>();
 
     // if ipv4 or ipv6 requested explicitly
     bool preferHostAddr = false;
@@ -968,11 +968,11 @@ int main(int argc, char **argv)
                 throw std::runtime_error("Unable to read server root certificate file");                                                
             return LBManager(uri, true, preferHostAddr, opts_res.value());                                                              
         }                                                                                                                             
-                                                                                                                                        
+
         if (novalidate)                                                                                                
             std::cerr << "Skipping server certificate validation" << std::endl;
 
-        return LBManager(uri, !novalidate, preferHostAddr);                                                                                    
+        return LBManager(uri, !novalidate, preferHostAddr);
     };  
 
     try {

bin/meson.build

@@ -30,4 +30,4 @@ executable('e2sar_udp_relay', 'e2sar_udp_relay.cpp',
             link_with: libe2sar,
             install: true,
             link_args: linker_flags,
-            dependencies: [boost_dep, thread_dep])
+            dependencies: [boost_dep, thread_dep, grpc_dep, protobuf_dep])