[Backport of #25403] fix(data-consumption): prevent hook usage counter from going negative - #25503
Conversation
The usage counter that tracks how many plugins are listening to a given subscription can reach negative values, which is invalid and likely indicates a bug in the unsubscribe logic. Extract the hook usage update logic into a dedicated function that guards against negative values and logs a warning when such an update is attempted.
WalkthroughThe plugin engine now centralizes hook usage count delta handling, logs warnings for underflow, and clamps negative results to zero across custom and general update paths. The HTML5 package now obtains Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8)bigbluebutton-html5/package.jsonTraceback (most recent call last): Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
bigbluebutton-html5/imports/ui/components/plugins-engine/data-consumption/utils.ts (1)
13-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd focused tests for the shared underflow contract.
Cover both update paths with
count = 0anddelta = -1; assert the count remains0and the warning includes the hook name, current count, and delta. Also cover normal positive and negative updates.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bigbluebutton-html5/imports/ui/components/plugins-engine/data-consumption/utils.ts` around lines 13 - 25, Add focused tests for applyHookUsageCountDelta covering underflow from count 0 with delta -1, asserting a returned count of 0 and a warning containing hookName, currentCount, and delta. Also test normal positive and negative deltas, preserving the expected arithmetic behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bigbluebutton-html5/package.json`:
- Line 82: Replace the mutable bigbluebutton-html-plugin-sdk PR URL in
package.json with an immutable committed SHA or published release version, then
regenerate the lockfile so it records the pinned dependency and resolved
tarball.
---
Nitpick comments:
In
`@bigbluebutton-html5/imports/ui/components/plugins-engine/data-consumption/utils.ts`:
- Around line 13-25: Add focused tests for applyHookUsageCountDelta covering
underflow from count 0 with delta -1, asserting a returned count of 0 and a
warning containing hookName, currentCount, and delta. Also test normal positive
and negative deltas, preserving the expected arithmetic behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7c6bee2f-ac4e-488a-a6e0-64652a1affb5
⛔ Files ignored due to path filters (1)
bigbluebutton-html5/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
bigbluebutton-html5/imports/ui/components/plugins-engine/data-consumption/utils.tsbigbluebutton-html5/package.json
| "autoprefixer": "^10.4.4", | ||
| "babel-runtime": "~6.26.0", | ||
| "bigbluebutton-html-plugin-sdk": "0.0.103", | ||
| "bigbluebutton-html-plugin-sdk": "https://codeload.github.com/bigbluebutton/bigbluebutton-html-plugin-sdk/tar.gz/refs/pull/279/head", |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== package.json relevant line =="
sed -n '78,86p' bigbluebutton-html5/package.json
echo
echo "== package-lock.json relevant entries =="
python3 - <<'PY'
import json, pathlib
p=pathlib.Path("bigbluebutton-html5/package-lock.json")
data=json.loads(p.read_text())
pkg=data.get("packages",{}).get("node_modules/bigbluebutton-html-plugin-sdk")
print(pkg)
for path,node in data.get("packages",{}).items():
if "bigbluebutton-html-plugin-sdk" in path or path == "":
if node and ("version" in node or "resolved" in node or "integrity" in node):
print(path, {"version":node.get("version"), "resolved":node.get("resolved"), "integrity":node.get("integrity")})
PY
echo
echo "== npm lockfile format / semver check (package manager manifests) =="
python3 - <<'PY'
import json
pkg=json.load(open("bigbluebutton-html5/package.json"))
lock=json.load(open("bigbluebutton-html5/package-lock.json"))
print("npm_version:", lock.get("lockfileVersion"), lock.get("lockfileVersion2"), lock.get("lockfileVersion3"))
print("require_lockfile:", lock.get("requiresLockfileGeneratedBy") or "")
print("devDependency:", pkg.get("devDependencies",{}).get("bigbluebutton-html-plugin-sdk"))
PYRepository: bigbluebutton/bigbluebutton
Length of output: 1509
🌐 Web query:
GitHub refs/pull/279/head tarball URL mutable PR head force push documentation
💡 Result:
GitHub pull request references, such as refs/pull/{number}/head, are read-only references created and managed by GitHub [1][2]. They are not standard branches in your repository; rather, they are special references that point to the latest commit on a pull request's head branch [1][2]. Because these are managed automatically by GitHub, they are considered mutable in that they update automatically to reflect the latest state of the associated pull request (e.g., when new commits are pushed to the PR branch) [1][2]. However, you cannot "force push" to them yourself through standard Git operations, nor can you directly update them via the GitHub REST API or Git command-line tools to point to arbitrary commits [3][1]. Any attempt to modify these specific internal refs via standard push mechanisms will be rejected by GitHub. When retrieving data via tarball or zipball URLs for these references, you are accessing the state of the pull request at that moment as defined by the ref. Because the ref itself is dynamic (mutable) and managed by GitHub, the content behind those URLs will change whenever the underlying pull request head is updated [1][2]. If you require a stable, immutable reference to a specific version of a pull request, you should always use the specific commit SHA (hash) rather than the refs/pull/{number}/head reference [4].
Citations:
- 1: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/about-pull-requests?platform=linux
- 2: https://github.com/github/docs/blob/main/content/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/about-pull-requests.md
- 3: https://docs.github.com/en/rest/git/refs?apiVersion=2022-11-28
- 4: https://www.kenmuse.com/blog/the-many-shas-of-a-github-pull-request/
Pin the SDK dependency to an immutable revision.
refs/pull/279/head points to the current PR HEAD, so any updated PR commit changes the tarball behind the dependency URL. Regenerate the lockfile and pin a committed SHA or published release version instead of keeping the mutable ref only in package.json.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bigbluebutton-html5/package.json` at line 82, Replace the mutable
bigbluebutton-html-plugin-sdk PR URL in package.json with an immutable committed
SHA or published release version, then regenerate the lockfile so it records the
pinned dependency and resolved tarball.
✅ Automated tests have passed! |
|
This pull request has conflicts ☹ |
2 participants
What does this PR do?
The usage counter that tracks how many plugins are listening to a given subscription can reach negative values, which is invalid and likely indicates a bug in the unsubscribe logic.
Extract the hook usage update logic into a dedicated function that guards against negative values and logs a warning when such an update is attempted.
Closes Issue(s)
Closes N/A
More
Closely related to:
useCustomSubscriptionstuck inloading: truestate bigbluebutton-html-plugin-sdk#275