You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@texera.apache.org by gi...@apache.org on 2026/08/23 07:01:02 UTC
(texera) 01/01: test(amber): cover the collaboration resource's editing-lock hand-off (#7863)
This is an automated email from the ASF dual-hosted git repository.
github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-7863-65f1f668acf38d83aa17991359477d3f2f44db3e
in repository https://gitbox.apache.org/repos/asf/texera.git
commit 79d084fa8d9d3f9be60317e09a3210f29adb2127
Author: Xinyuan Lin <xi...@uci.edu>
AuthorDate: Sun Aug 23 07:00:40 2026 +0000
test(amber): cover the collaboration resource's editing-lock hand-off (#7863)
### What changes were proposed in this PR?
`CollaborationResourceSpec` goes from 16 tests to 28, covering the parts
of `CollaborationResource` that decide who holds the editing lock.
Measured with two `WorkflowExecutionService/jacoco` runs, one fresh sbt
JVM each, `rm -rf` on the jacoco dir before each, and an identical FQCN
suite-name filter in both — never a `-z` phrase filter.
`TEXERA_SERVICE_LOG_LEVEL=WARN` to match CI. The "before" spec came from
`git show HEAD:<path>`.
| Metric | Before | After |
|---|---|---|
| Codecov (fully-covered lines) | 67/87 = 77.0% | **74/87 = 85.1%** |
| JaCoCo line-hit | 4 of 87 missed | **0 missed = 100%** |
| Branch arms | 27 of 74 missed (63.5% covered) | **15 of 74 missed
(79.7%)** |
The before run reproduces the reported "4 missed + 16 partial of 87,
77.0%, 63.51% branch" figure for figure, so this is the line map Codecov
sees.
New tests cover a session that re-registers on a different wid, the lock
holder not handing the lock to a read-only peer, exactly one of several
writable peers receiving it, and sessions sitting on a different
workflow being ignored.
### Verification
18 mutations, **17 killed, 1 alive.** Nine of the seventeen are killed
by exactly one test each.
**The first draft claimed "no surviving mutants". That was wrong, and
six survivors were confirmed empirically** — each re-run against the
pre-review spec and each coming back `24 succeeded, 0 failed`. Five are
now dead; the sixth is reported below. Four of the six existed because a
test asserted something other than what its name claimed:
- A test named "hand the lock to a remaining **writable** peer" pinned
nothing about writability: both sessions authenticated as the same uid,
so the privilege predicate was unobservable. Fixed by seeding a
genuinely read-only second peer and asserting it does *not* receive the
lock.
- Flipping the once-only granting guard survived, because a single peer
in the bucket can only prove the latch *starts* false. That row is now
correctly described as evidence for the latch's initial value, and
once-only granting is pinned by a separate multi-peer test.
- "Should leave a null holder sentinel alone" pinned only Scala's
null-safe `==` — every assertion also passed on the inverted branch.
Rewritten.
- `peer.getId shouldBe "s2"` was a ScalaMock echo of `(() =>
session.getId).expects().returning("s2")`, asserting the fixture rather
than production. Removed.
**The surviving mutant, stated plainly:** substituting
`sessionIdWIdMap(sessId)` for the passed `wId` in the read-only check at
line 188 survives all 28 tests. It is equivalent over every state the
class's own bookkeeping invariant permits, since the registration paths
write that wid themselves. Killing it would require constructing a state
the class cannot reach.
### This repair pass added zero coverage, and that should not be misread
The pre-review 24-test spec already measured 74/87 = 85.1% with 15/74
arms missed; the 28-test spec measures byte-identically the same. The
four added tests earn no tracked lines because the hand-off loop body
compiles to `ACC_SYNTHETIC` `$anonfun` methods that JaCoCo's
`SyntheticFilter` drops — `jacoco.xml` has no `<line>` entry for 188-195
at all. They buy mutation strength, not lines.
### Deliberately not included
13 lines remain partial and none were chased: logging guards, a lazy-val
bitmap, `MatchError` arms, dead short-circuit arms on line 122, and
null-`Session.getId` arms on 135 and 181. None of them is a surviving
mutant — they are structurally unreachable.
Stated as a gap rather than a claim: the `CommandRequest` /
`RestoreVersionRequest` fan-out loops got no mutations in either pass.
Their bodies are synthetic `$anonfun` and earn zero coverage lines, and
scope was not expanded into them.
### Reported, not pinned
Two defects are named in the spec rather than asserted: the
`AcquireLockRequest` null-sentinel handling, and a stale-bucket leak. A
pre-existing test already cements the first as intended behaviour, which
is worth knowing before anyone changes it.
No production file is touched — `git diff -- '*/src/main/*'` was checked
after all 24 mutation reverts and again at the end, 0 bytes each time,
with the production sha256 byte-identical to the pre-mutation snapshot.
### Any related issues, documentation, discussions?
Closes #7861
### How was this PR tested?
```
sbt "WorkflowExecutionService/testOnly org.apache.texera.web.resource.CollaborationResourceSpec"
```
```
[info] Total number of tests run: 28
[info] Suites: completed 1, aborted 0
[info] Tests: succeeded 28, failed 0, canceled 0, ignored 0, pending 0
```
The test-report XML independently counts 28 `testcase` entries with no
`failure` or `error`. `Test/scalafmtCheck` and `Test/scalafix --check`
both pass.
### Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 5)
---------
Signed-off-by: Xinyuan Lin <xi...@uci.edu>
Co-authored-by: Copilot Autofix powered by AI <17...@users.noreply.github.com>
---
.../web/resource/CollaborationResourceSpec.scala | 258 ++++++++++++++++++++-
1 file changed, 247 insertions(+), 11 deletions(-)
diff --git a/amber/src/test/scala/org/apache/texera/web/resource/CollaborationResourceSpec.scala b/amber/src/test/scala/org/apache/texera/web/resource/CollaborationResourceSpec.scala
index a313a667a2..359f3a3bdc 100644
--- a/amber/src/test/scala/org/apache/texera/web/resource/CollaborationResourceSpec.scala
+++ b/amber/src/test/scala/org/apache/texera/web/resource/CollaborationResourceSpec.scala
@@ -46,7 +46,19 @@ import scala.collection.mutable.ArrayBuffer
// with no external collaborators; the TryLockRequest cases that reach
// WorkflowAccessResource.hasWriteAccess additionally mix in MockTexeraDB and
// seed a workflow_user_access row so the privilege check reads a real value.
-// The lock hand-off inside myOnClose still needs SqlServer and is left uncovered.
+// The lock hand-off inside myOnClose consults the same privilege check, so some
+// of the myOnClose cases at the bottom of this file also mix in MockTexeraDB.
+// When the candidate privilege needs to be observable, they seed two users (one
+// WRITE, one READ) on the same workflow.
+//
+// Two defects in this class are deliberately NOT pinned here, because pinning
+// them would cement them: (1) a session that sends WIdRequest twice is added to
+// the new wid's bucket but never removed from the old one, leaving a stale
+// hand-off candidate that outlives the session; (2) AcquireLockRequest
+// dereferences the `null` holder sentinel that TryLockRequest writes, so it
+// throws where TryLockRequest copes. Case (2) predates this spec and is pinned
+// by "AcquireLockRequest should rethrow when the holder slot holds the null
+// sentinel" as current behaviour, not as desired behaviour.
class CollaborationResourceSpec
extends AnyFlatSpec
with Matchers
@@ -62,6 +74,10 @@ class CollaborationResourceSpec
// WorkflowAccessResource.hasWriteAccess reads a real privilege.
private val accessUid = 8201
private val accessWid = 8301
+ // A second user on the SAME workflow, seeded with READ only. Needed because
+ // the hand-off loop in myOnClose consults checkIsReadOnly per candidate
+ // session: with one uid the privilege predicate cannot be observed at all.
+ private val readOnlyUid = 8202
override protected def beforeAll(): Unit = {
initializeDBAndReplaceDSLContext()
@@ -92,12 +108,6 @@ class CollaborationResourceSpec
// the given privilege, so WorkflowAccessResource.getPrivilege(accessWid,
// accessUid) returns it.
private def seedAccess(privilege: PrivilegeEnum): Unit = {
- val user = new User
- user.setUid(Integer.valueOf(accessUid))
- user.setName("collab_lock_user")
- user.setRole(UserRoleEnum.REGULAR)
- new UserDao(getDSLContext.configuration()).insert(user)
-
val workflow = new Workflow
workflow.setWid(Integer.valueOf(accessWid))
workflow.setName("collab-lock-wf")
@@ -107,9 +117,21 @@ class CollaborationResourceSpec
workflow.setLastModifiedTime(new Timestamp(System.currentTimeMillis()))
new WorkflowDao(getDSLContext.configuration()).insert(workflow)
+ seedUserAccess(accessUid, "collab_lock_user", privilege)
+ }
+
+ // Adds one more (uid, accessWid) row. WORKFLOW_USER_ACCESS is keyed on the
+ // pair, so several users can hold different privileges on one workflow.
+ private def seedUserAccess(uid: Int, name: String, privilege: PrivilegeEnum): Unit = {
+ val user = new User
+ user.setUid(Integer.valueOf(uid))
+ user.setName(name)
+ user.setRole(UserRoleEnum.REGULAR)
+ new UserDao(getDSLContext.configuration()).insert(user)
+
new WorkflowUserAccessDao(getDSLContext.configuration())
.insert(
- new WorkflowUserAccess(Integer.valueOf(accessUid), Integer.valueOf(accessWid), privilege)
+ new WorkflowUserAccess(Integer.valueOf(uid), Integer.valueOf(accessWid), privilege)
)
}
@@ -120,11 +142,16 @@ class CollaborationResourceSpec
.execute()
getDSLContext.deleteFrom(WORKFLOW).where(WORKFLOW.WID.eq(accessWid)).execute()
getDSLContext.deleteFrom(USER).where(USER.UID.eq(accessUid)).execute()
+ getDSLContext.deleteFrom(USER).where(USER.UID.eq(readOnlyUid)).execute()
}
- // A session already registered on accessWid as accessUid, ready to TryLock.
- private def lockingSession(id: String): (Session, ArrayBuffer[String]) = {
- val (session, sent) = mockSession(id, uId = Some(accessUid))
+ // A session already registered on accessWid, ready to TryLock. Defaults to
+ // accessUid; pass readOnlyUid for a session that cannot write the workflow.
+ private def lockingSession(
+ id: String,
+ uId: Int = accessUid
+ ): (Session, ArrayBuffer[String]) = {
+ val (session, sent) = mockSession(id, uId = Some(uId))
resource.myOnOpen(session)
resource.myOnMsg(session, send(WIdRequest(accessWid)))
sent.clear()
@@ -240,6 +267,23 @@ class CollaborationResourceSpec
wIdSessionIdsMap(7) should contain theSameElementsAs Set("s1", "s2")
}
+ it should "update the recorded wid when a session re-registers" in {
+ val (session, _) = mockSession("s1", uId = Some(42))
+ resource.myOnOpen(session)
+
+ resource.myOnMsg(session, send(WIdRequest(7)))
+ resource.myOnMsg(session, send(WIdRequest(8)))
+
+ // The second request replaces the recorded wid rather than being ignored.
+ sessionIdWIdMap("s1") shouldBe 8
+ wIdSessionIdsMap(8) should contain only "s1"
+ // Deliberately NOT asserted: what wIdSessionIdsMap(7) holds afterwards.
+ // The handler never removes the session from the bucket it left, so the
+ // stale entry survives and stays a hand-off candidate for wid 7 -- a real
+ // defect (see the class comment). Pinning today's value here would make
+ // that leak harder to fix, so this test stays silent about it.
+ }
+
// -- fan-out ----------------------------------------------------------------
/**
@@ -320,6 +364,10 @@ class CollaborationResourceSpec
sent.head should include("WorkflowAccessEvent")
sent.head should include("\"workflowReadonly\":false")
sent(1) should include("LockGrantedEvent")
+ // The DUMMY_WID fast path answers the sender WITHOUT taking ownership of
+ // the holder slot. Every anonymous/reconnecting session shares wid -1, so
+ // recording a holder there would make unrelated clients contend for it.
+ wIdLockHolderSessionIdMap should not contain key(DUMMY_WID)
}
"AcquireLockRequest" should "hand the lock over from the previous holder" in {
@@ -411,4 +459,192 @@ class CollaborationResourceSpec
sent(1) should include("LockRejectedEvent")
wIdLockHolderSessionIdMap(accessWid) shouldBe "other-session" // unchanged
}
+
+ "TryLockRequest from a read-only user" should "leave an existing holder entry alone" in {
+ seedAccess(PrivilegeEnum.READ)
+ val (session, sent) = lockingSession("s1")
+ // Somebody already holds the lock; a read-only viewer arriving afterwards
+ // must not overwrite the holder slot with the "nobody holds it" sentinel.
+ wIdLockHolderSessionIdMap(accessWid) = "someone-else"
+
+ resource.myOnMsg(session, send(TryLockRequest()))
+
+ sent should have size 2
+ sent.head should include("LockRejectedEvent")
+ sent(1) should include("WorkflowAccessEvent")
+ sent(1) should include("\"workflowReadonly\":true")
+ wIdLockHolderSessionIdMap(accessWid) shouldBe "someone-else"
+ }
+
+ "TryLockRequest from a writable user" should "grant the lock over the null sentinel" in {
+ seedAccess(PrivilegeEnum.WRITE)
+ val (session, sent) = lockingSession("s1")
+ // The key exists but holds `null`, i.e. a read-only viewer has been here
+ // and nobody holds the lock. A writable user must still get it.
+ wIdLockHolderSessionIdMap(accessWid) = null
+
+ resource.myOnMsg(session, send(TryLockRequest()))
+
+ sent should have size 2
+ sent.head should include("WorkflowAccessEvent")
+ sent.head should include("\"workflowReadonly\":false")
+ sent(1) should include("LockGrantedEvent")
+ wIdLockHolderSessionIdMap(accessWid) shouldBe "s1"
+ }
+
+ it should "re-grant the lock to the session that already holds it" in {
+ seedAccess(PrivilegeEnum.WRITE)
+ val (session, sent) = lockingSession("s1")
+ wIdLockHolderSessionIdMap(accessWid) = "s1"
+
+ resource.myOnMsg(session, send(TryLockRequest()))
+
+ sent should have size 2
+ sent.head should include("WorkflowAccessEvent")
+ sent.head should include("\"workflowReadonly\":false")
+ sent(1) should include("LockGrantedEvent")
+ wIdLockHolderSessionIdMap(accessWid) shouldBe "s1"
+ }
+
+ // -- myOnClose releasing the lock -------------------------------------------
+
+ "myOnClose" should "tolerate a workflow whose session bucket has gone" in {
+ val (session, _) = mockSession("s1", uId = Some(1))
+ resource.myOnOpen(session)
+ resource.myOnMsg(session, send(WIdRequest(7)))
+ // The guard on wIdSessionIdsMap is what keeps the two maps from having to
+ // agree; without it this close would blow up on a missing key.
+ wIdSessionIdsMap.remove(7)
+
+ noException should be thrownBy resource.myOnClose(session)
+
+ sessionIdWIdMap should not contain key("s1")
+ sessionIdSessionMap should not contain key("s1")
+ }
+
+ it should "leave a null holder sentinel alone" in {
+ seedAccess(PrivilegeEnum.WRITE)
+ // `null` is the "nobody holds it" sentinel; comparing it against the
+ // departing session id must not dereference it, and must not be mistaken
+ // for a match. A WRITABLE PEER is deliberately left in the bucket: without
+ // one, "branch skipped" and "branch entered" are indistinguishable,
+ // because entering it would only rewrite null over null and find nobody to
+ // grant to. With s2 present, entering it hands s2 the lock.
+ val (session, _) = lockingSession("s1")
+ val (_, peerSent) = lockingSession("s2")
+ wIdLockHolderSessionIdMap(accessWid) = null
+ peerSent.clear()
+
+ noException should be thrownBy resource.myOnClose(session)
+
+ wIdLockHolderSessionIdMap should contain key accessWid
+ wIdLockHolderSessionIdMap(accessWid) shouldBe null
+ peerSent shouldBe empty
+ }
+
+ "myOnClose by the lock holder" should "hand the lock to a remaining writable peer" in {
+ seedAccess(PrivilegeEnum.WRITE)
+ // Two sessions of the same user, e.g. two browser tabs, both writable.
+ val (holder, _) = lockingSession("s1")
+ val (_, peerSent) = lockingSession("s2")
+ wIdLockHolderSessionIdMap(accessWid) = "s1"
+ peerSent.clear()
+
+ resource.myOnClose(holder)
+
+ wIdLockHolderSessionIdMap(accessWid) shouldBe "s2"
+ peerSent should have size 1
+ peerSent.head should include("LockGrantedEvent")
+ wIdSessionIdsMap(accessWid) should contain only "s2"
+ }
+
+ it should "not hand the lock to a read-only peer" in {
+ seedAccess(PrivilegeEnum.WRITE)
+ seedUserAccess(readOnlyUid, "collab_lock_reader", PrivilegeEnum.READ)
+ val (holder, _) = lockingSession("s1")
+ // The only session left behind belongs to a user with READ only.
+ val (_, readerSent) = lockingSession("s2", uId = readOnlyUid)
+ wIdLockHolderSessionIdMap(accessWid) = "s1"
+ readerSent.clear()
+
+ resource.myOnClose(holder)
+
+ // Nobody eligible remains, so the lock stays unheld. This is what makes
+ // the privilege check in the hand-off loop observable: it is checked for
+ // the CANDIDATE, not for the departing holder.
+ wIdLockHolderSessionIdMap(accessWid) shouldBe null
+ readerSent shouldBe empty
+ }
+
+ it should "grant the lock to exactly one of several writable peers" in {
+ seedAccess(PrivilegeEnum.WRITE)
+ val (holder, _) = lockingSession("s1")
+ val (_, peer2Sent) = lockingSession("s2")
+ val (_, peer3Sent) = lockingSession("s3")
+ wIdLockHolderSessionIdMap(accessWid) = "s1"
+ peer2Sent.clear()
+ peer3Sent.clear()
+
+ resource.myOnClose(holder)
+
+ // The once-only latch must stop the loop after the first eligible peer;
+ // two grants would leave two clients each believing they hold the lock.
+ // Asserted order-agnostically because the bucket is an unordered Set.
+ (peer2Sent.size + peer3Sent.size) shouldBe 1
+ val winner = wIdLockHolderSessionIdMap(accessWid)
+ Set("s2", "s3") should contain(winner)
+ val winnerSent = if (winner == "s2") peer2Sent else peer3Sent
+ winnerSent should have size 1
+ winnerSent.head should include("LockGrantedEvent")
+ }
+
+ it should "ignore sessions sitting on a different workflow" in {
+ seedAccess(PrivilegeEnum.WRITE)
+ val (holder, _) = lockingSession("s1")
+ // Same writable user, but registered on another workflow, so it is not a
+ // candidate for accessWid's lock even though its session is still open.
+ val (other, otherSent) = mockSession("s3", uId = Some(accessUid))
+ resource.myOnOpen(other)
+ resource.myOnMsg(other, send(WIdRequest(9999)))
+ wIdLockHolderSessionIdMap(accessWid) = "s1"
+ otherSent.clear()
+
+ resource.myOnClose(holder)
+
+ // The hand-off must read the departing workflow's own bucket, which is now
+ // empty -- not the set of every open session on the server.
+ wIdLockHolderSessionIdMap(accessWid) shouldBe null
+ otherSent shouldBe empty
+ }
+
+ it should "clear the holder to the null sentinel when no session is left" in {
+ val (session, sent) = mockSession("s1", uId = Some(1))
+ resource.myOnOpen(session)
+ resource.myOnMsg(session, send(WIdRequest(7)))
+ wIdLockHolderSessionIdMap(7) = "s1"
+ sent.clear()
+
+ resource.myOnClose(session)
+
+ wIdLockHolderSessionIdMap(7) shouldBe null
+ wIdSessionIdsMap(7) shouldBe empty
+ sent shouldBe empty
+ }
+
+ "myOnClose by a non-holder" should "leave the lock with its holder" in {
+ val (first, _) = mockSession("s1", uId = Some(1))
+ val (second, secondSent) = mockSession("s2", uId = Some(2))
+ resource.myOnOpen(first)
+ resource.myOnOpen(second)
+ resource.myOnMsg(first, send(WIdRequest(7)))
+ resource.myOnMsg(second, send(WIdRequest(7)))
+ wIdLockHolderSessionIdMap(7) = "s2"
+ secondSent.clear()
+
+ resource.myOnClose(first)
+
+ wIdLockHolderSessionIdMap(7) shouldBe "s2"
+ secondSent shouldBe empty
+ wIdSessionIdsMap(7) should contain only "s2"
+ }
}