I'm still learning about this project, and I received help from an AI. See if this works:
Quest Completion Race Condition Fix
Problem
When a player clicks the "Complete Quest" button multiple times quickly, the server processes all concurrent requests in parallel. Multiple threads simultaneously pass the !isStarted() guard check in QuestState.exitQuest() (since the state is still STARTED for all of them), and each grants the full quest reward — resulting in players receiving the reward 5–10 times.
Root Cause
In
QuestState.java
, the private method
exitQuest(boolean repeatable)
(line 777) does:
java
if (!isStarted()) {
return; // guard check
}
// ... removes items, gives rewards, changes state to COMPLETED
There is no synchronization between the guard check and the state change. Two threads can both pass !isStarted() at the same time before either has changed _state to COMPLETED.
Proposed Changes
QuestState Core
[MODIFY]
QuestState.java
Add a synchronized(this) block to the private
exitQuest(boolean repeatable)
method so that the check-and-set of the quest state is atomic per
QuestState
instance.
diff
private void exitQuest(boolean repeatable)
{
if (_simulated)
{
return;
}
_player.removeNotifyQuestOfDeath(this);
- if (!isStarted())
- {
- return;
- }
+ synchronized (this)
+ {
+ if (!isStarted())
+ {
+ return;
+ }
+ // Mark as no longer started immediately to block re-entry
+ if (!repeatable)
+ {
+ _state = State.COMPLETED;
+ }
+ else
+ {
+ _state = State.CREATED; // temporary sentinel so re-entry fails
+ }
+ }
Wait — this approach is incomplete because after marking state we still need to remove items and save to DB. A cleaner approach is to use synchronized(this) around only the guard + state flip:
The safest, cleanest fix is wrapping the entire body of
exitQuest(boolean repeatable)
in synchronized (this), so only one thread can execute it at a time per quest state object:
java
private void exitQuest(boolean repeatable)
{
if (_simulated)
{
return;
}
synchronized (this)
{
_player.removeNotifyQuestOfDeath(this);
if (!isStarted())
{
return;
}
// Clean registered quest items
getQuest().removeRegisteredQuestItems(_player);
Quest.deleteQuestInDb(this, repeatable);
if (repeatable)
{
_player.delQuestState(_questName);
_player.sendPacket(new QuestList(_player));
}
else
{
setState(State.COMPLETED);
}
_vars = null;
}
}
NOTE
synchronized(this) uses the
QuestState
object as the monitor. Since each player has one
QuestState
per quest, this ensures only one thread can complete a given quest for a given player at a time. Other players' quests are completely unaffected (they use different
QuestState
objects).
Verification Plan
Automated Tests
No automated test suite was found that covers quest state logic. Manual in-game testing is the recommended verification approach.
Manual Verification
Start the game server in dev mode
Pick any quest and complete its objectives on a test character
Talk to the quest NPC and open the completion dialog
Click the "Complete Quest" button rapidly multiple times (5–10 clicks within 1 second)
Expected result: Only one reward is received (visible in inventory and system messages)
Before fix: Multiple copies of the reward would appear in inventory
IMPORTANT
This can also be simulated by sending multiple
RequestBypassToServer
packets with the quest completion bypass command programmatically (e.g., via a packet injector or custom client). Both approaches verify the same race condition path.
Quest Completion Race Condition — Fix Walkthrough
What was done:
1 file modified: QuestState.java
The private method `exitQuest(boolean repeatable)`
(line 777) received a `synchronized(this)` block around its entire logical body.
Diff applied
1
1
/*
2
2
* Copyright (c) 2013 L2jMobius
3
3
*
⋯ Expand 777 more lines
781
781
return;
782
782
}
783
783
784
_player.removeNotifyQuestOfDeath(this);
785
786
if (!isStarted())
784
synchronized (this)
787
785
{
788
return;
786
_player.removeNotifyQuestOfDeath(this);
787
788
if (!isStarted())
789
{
790
return;
791
}
792
793
// Clean registered quest items
794
getQuest().removeRegisteredQuestItems(_player);
795
796
Quest.deleteQuestInDb(this, repeatable);
797
if (repeatable)
798
{
799
_player.delQuestState(_questName);
800
_player.sendPacket(new QuestList(_player));
801
}
802
else
803
{
804
setState(State.COMPLETED);
805
}
806
807
_vars = null;
789
808
}
790
791
// Clean registered quest items
792
getQuest().removeRegisteredQuestItems(_player);
793
794
Quest.deleteQuestInDb(this, repeatable);
795
if (repeatable)
796
{
797
_player.delQuestState(_questName);
798
_player.sendPacket(new QuestList(_player));
799
}
800
else
801
{
802
setState(State.COMPLETED);
803
}
804
805
_vars = null;
806
809
}
807
810
808
811
/**
⋯ Expand 61 more lines
870
873
_simulated = simulated;
871
874
}
872
875
}
Why it works
Situation Before After
Player clicks 1x Quest completes normally ✅ Quest completes normally ✅
Player clicks 5-10x quickly 5-10 threads pass the !isStarted() guard and deliver multiple rewards ❌ Only 1 thread enters the synchronized state. The others wait in the queue and when they enter they find _state == COMPLETED, the !isStarted() passes and they return immediately without reward ✅
The synchronized monitor is the QuestState object itself
—which is unique per player per quest. This means:
No performance impact for other players (each has their own QuestState)
No deadlock possible (the lock is acquired and released in the same method)
Works for normal, repeatable, and daily quests (all go through the same internal method)
How to test:
Pick up any quest and complete the objectives.
Open the dialogue with the NPC to complete the quest.
Click the completion button 5-10 times quickly.
Check your inventory — the reward should only appear once.