app_queue: fix double increment of member->calls with shared_lastcall

Under high concurrency, update_queue() may be invoked multiple times
for the same call, causing member->calls and queue-level counters to
be incremented more than once.

The existing starttime check is not atomic and allows concurrent
execution paths to pass. Treat member->starttime as a single-use token
and consume it via CAS to ensure the call is counted exactly once.

This also prevents incorrect call distribution when using strategies
such as fewestcalls.

Observed as a regression after upgrading to 20.17.

Resolves: #1736

diff --git a/apps/app_queue.c b/apps/app_queue.c
index 06fc4b0ac11ee02d09cab94473bccb576d8fd787..b49c7a09220896c521cc7e585bb319ab8bebd758 100644 (file)
--- a/apps/app_queue.c
+++ b/apps/app_queue.c
@@ -6162,36 +6162,48 @@ static int wait_our_turn(struct queue_ent *qe, int ringing, enum queue_result *r
  * \brief update the queue status
  * \retval 0 always
 */
-static int update_queue(struct call_queue *q, struct member *member, int callcompletedinsl, time_t starttime)
+static int update_queue(struct call_queue *q, struct member *member,
+       int callcompletedinsl, time_t starttime)
 {
        int oldtalktime;
-       int newtalktime = time(NULL) - starttime;
+       int newtalktime;
        struct member *mem;
        struct call_queue *qtmp;
        struct ao2_iterator queue_iter;
+       int did_increment_any = 0;
+
+       if (!starttime) {
+               return 0;
+       }
+
+       newtalktime = (int)(time(NULL) - starttime);
 
        /* It is possible for us to be called when a call has already been considered terminated
         * and data updated, so to ensure we only act on the call that the agent is currently in
         * we check when the call was bridged.
         */
-       if (!starttime || (member->starttime != starttime)) {
+       ao2_lock(q);
+       if (member->starttime != starttime) {
+               ao2_unlock(q);
                return 0;
        }
+       member->starttime = 0;
+       ao2_unlock(q);
 
        if (shared_lastcall) {
                queue_iter = ao2_iterator_init(queues, 0);
-               while ((qtmp = ao2_t_iterator_next(&queue_iter, "Iterate through queues"))) {
+               while ((qtmp = ao2_t_iterator_next(&queue_iter, "Iterate queues"))) {
                        ao2_lock(qtmp);
                        if ((mem = ao2_find(qtmp->members, member, OBJ_POINTER))) {
                                time(&mem->lastcall);
                                mem->calls++;
                                mem->callcompletedinsl = 0;
-                               mem->starttime = 0;
                                mem->lastqueue = q;
+                               did_increment_any = 1;
                                ao2_ref(mem, -1);
                        }
                        ao2_unlock(qtmp);
-                       queue_t_unref(qtmp, "Done with iterator");
+                       queue_t_unref(qtmp, "queue iteration done");
                }
                ao2_iterator_destroy(&queue_iter);
        } else {
@@ -6199,8 +6211,8 @@ static int update_queue(struct call_queue *q, struct member *member, int callcom
                time(&member->lastcall);
                member->callcompletedinsl = 0;
                member->calls++;
-               member->starttime = 0;
                member->lastqueue = q;
+               did_increment_any = 1;
                ao2_unlock(q);
        }
        /* Member might never experience any direct status change (local
@@ -6210,22 +6222,24 @@ static int update_queue(struct call_queue *q, struct member *member, int callcom
         */
        pending_members_remove(member);
 
-       ao2_lock(q);
-       q->callscompleted++;
-       if (callcompletedinsl) {
-               q->callscompletedinsl++;
-       }
-       if (q->callscompleted == 1) {
-               q->talktime = newtalktime;
-       } else {
-               /* Calculate talktime using the same exponential average as holdtime code */
-               oldtalktime = q->talktime;
-               q->talktime = (((oldtalktime << 2) - oldtalktime) + newtalktime) >> 2;
+       if (did_increment_any) {
+               ao2_lock(q);
+               q->callscompleted++;
+               if (callcompletedinsl) {
+                       q->callscompletedinsl++;
+               }
+               if (q->callscompleted == 1) {
+                       q->talktime = newtalktime;
+               } else {
+                       /* Calculate talktime using the same exponential average as holdtime code */
+                       oldtalktime = q->talktime;
+                       q->talktime = (((oldtalktime << 2) - oldtalktime) + newtalktime) >> 2;
+               }
+               ao2_unlock(q);
        }
-       ao2_unlock(q);
+
        return 0;
 }
-
 /*! \brief Calculate the metric of each member in the outgoing callattempts
  *
  * A numeric metric is given to each member depending on the ring strategy used