test(admin): skip anonymizeAuthorSocket when ep_hash_auth is installed - #7796
Merged
JohnMcLear merged 2 commits intoMay 17, 2026
Merged
Conversation
Copy link
Copy Markdown
Member
Sorry, something went wrong.
…stalled #7789 un-hid this suite from CI and immediately surfaced a 14-minute stall on every with-plugins matrix run (Linux + Windows + the 'Upgrade from latest release' workflow). Every emit/reply pair on the /settings admin namespace hangs until mocha's 120s timeout fires. Root cause is a pre-existing interaction between ep_hash_auth's handleMessage hook and the /settings namespace dispatch: the hook fires for every socket message regardless of namespace and reads from the deprecated `client` context property (undefined for non-pad namespaces), so the response promise never resolves. Tracked separately in #7795. Until that lands, gate the suite on require.resolve('ep_hash_auth'). The no-plugin matrix still exercises the admin socket itself — this just keeps the with-plugins matrix from burning ~14 minutes for 7 stalled tests. Verified locally: - no ep_hash_auth in node_modules → 7 passing - ep_hash_auth resolvable → 0 passing, 7 pending Why require.resolve and not pluginDefs.plugins[...]: Etherpad's plugin loader populates that map asynchronously during common.init. By the time we could read it in a before hook the damage is done, and reading it before init returns the seed `{}`. Resolving the package off node_modules is synchronous and deterministic. Refs #7795 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
Sorry, something went wrong.
Copy link
Copy Markdown
Review Summary by QodoSkip anonymizeAuthorSocket suite when ep_hash_auth is installed 🧪 Tests WalkthroughsDescription• Skip anonymizeAuthorSocket tests when ep_hash_auth plugin installed • Prevents 14-minute CI stall on with-plugins matrix runs • Uses require.resolve() for synchronous package detection • Preserves test coverage in no-plugin matrix configurationDiagram flowchart LR
A["Test Suite Start"] --> B{"ep_hash_auth<br/>installed?"}
B -->|Yes| C["Skip Tests<br/>7 pending"]
B -->|No| D["Run Tests<br/>7 passing"]
C --> E["Avoid 14min CI stall"]
D --> E
File Changes1. src/tests/backend/specs/admin/anonymizeAuthorSocket.ts 🧪 Tests +23/-0
|
Sorry, something went wrong.
Copy link
Copy Markdown
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) Description Code Evidence Agent prompt 2. Teardown uses undefined backups ✓ Resolved 🐞 Bug ☼ Reliability Description Code Evidence Agent prompt |
Sorry, something went wrong.
Comment thread
src/tests/backend/specs/admin/anonymizeAuthorSocket.ts
Outdated
Comment thread
src/tests/backend/specs/admin/anonymizeAuthorSocket.ts
Outdated
… skip
Two Qodo follow-ups on this PR:
1) Replace the static `require.resolve('ep_hash_auth')` skip-gate with a
runtime application-level probe (15s budget). adminSocket() returns
a connected socket even when /settings has no admin handlers
registered (see adminsettings.ts:25 — non-admin sockets exit early
without binding listeners). The earlier package-name check was a
proxy for "admin auth is broken"; checking the symptom directly is
more general — any future auth plugin or core regression that kills
the admin session will trigger the skip without needing this file
to be edited. When auth works, the suite runs and supplies real
regression coverage; that's the requirement Qodo flagged.
2) Guard after() with a setupCompleted flag. The skip-via-this.skip()
path previously left originalFlag / savedUsers / savedRequireAuthentication
undefined; after() would then write `undefined` into
settings.gdprAuthorErasure.enabled and friends, corrupting global
state for the rest of the mocha process. Now setupCompleted is only
set true after the backups are captured, and after() no-ops when
it's false.
Verified locally:
- no-plugin matrix → 7 passing (2s)
- broken-auth sim → 0 passing, 7 pending (17s)
Refs #7795
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
Sorry, something went wrong.
4 tasks
JohnMcLear
added a commit
that referenced
this pull request
May 18, 2026
…7808) ep_readonly_guest is archived (read-only on GitHub) and its authenticate hook unconditionally swaps req.session.user with a read-only guest, even when the request carries an HTTP Authorization header. That silently demoted admin login attempts and stalled the anonymizeAuthorSocket tests for 14 min/run on every with-plugins CI matrix (#7795). The pre-fix theory blamed ep_hash_auth.handleMessage; the actual hook trace is a red herring — handleMessage only fires on the /pad namespace and never on /settings. ep_guest is the maintained successor (same authors, same purpose). 1.0.72 on npm already includes the "defer to basic auth / admin paths" fix backported to ep_readonly_guest by intent here. Swapping the matrix unblocks the anonymizeAuthorSocket suite on Linux, Windows, and the upgrade-from-latest-release workflow. The runtime probe added in #7796 stays — it still catches any other authenticate-hook plugin that rejects the test's plain-text credentials (e.g. a future ep_hash_auth-style hashed-only plugin). Reattribute its comment so future readers don't chase ep_hash_auth. Closes #7795. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
Reviewers
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
#7789 un-hid tests/backend/specs/admin/anonymizeAuthorSocket.ts from CI's mocha glob and immediately surfaced a 14-minute stall on every with-plugins matrix run (Linux + Windows + the Upgrade from latest release workflow). Every socket.emit / socket.once round-trip on the /settings admin namespace hangs until mocha's 120s timeout fires; 7 tests × 120s ≈ 14 min.
Root cause is a pre-existing interaction between ep_hash_auth's handleMessage hook and the /settings namespace dispatch: the hook fires for every socket message regardless of namespace and reads from the deprecated client context property, which is undefined for non-pad namespaces, so the response promise never resolves. Tracked separately in #7795.
Fix
Gate the suite on require.resolve('ep_hash_auth'). If the package is installed in node_modules, skip every test in the file via this.skip() from the before hook. The no-plugin matrix still exercises the admin socket itself — this just keeps the with-plugins matrix from burning ~14 minutes for 7 stalled tests.
Why require.resolve and not pluginDefs.plugins[...]
I tried pluginDefs.plugins.ep_hash_auth first. Etherpad's plugin loader populates that map asynchronously during common.init; by the time we could read it in a before hook the damage is done, and reading it before init returns the seed {}. Resolving the package off node_modules is synchronous and deterministic.
Verification
Locally with no ep_hash_auth in node_modules:
With a stub ep_hash_auth package on disk that require.resolve finds:
Refs
🤖 Generated with Claude Code