commit 2d3413e76e6c44f10808a2d31be31ea65f4ae6bb
parent 38a83ebd191d19d6c4ffc41023000147758c8483
Author: Christian Grothoff <christian@grothoff.org>
Date: Thu, 6 Aug 2026 09:58:03 +0200
ensue serialization failures are managed with retries
Diffstat:
4 files changed, 235 insertions(+), 52 deletions(-)
diff --git a/src/challenger/challenger-httpd_common.c b/src/challenger/challenger-httpd_common.c
@@ -29,6 +29,11 @@
*/
#define RFC_8959_PREFIX "secret-token:"
+/**
+ * Maximum number of retries for the database interaction.
+ */
+#define MAX_RETRIES 3
+
// FIXME: enums would be nicer...
int
@@ -208,35 +213,51 @@ CH_build_full_redirect_url (
enum GNUNET_DB_QueryStatus qs;
enum MHD_Result ret;
- qs = CHALLENGERDB_get_validation (CH_context,
- nonce,
- &client_secret,
- &address,
- &client_scope,
- &client_state,
- &client_redirect_uri);
- switch (qs)
+ /* The session is SERIALIZABLE, so a serialization failure is a routine
+ outcome here; this runs on the success path of /solve and must not
+ turn into a 500 for a user who already passed the challenge. */
+ for (unsigned int r = 0; r<MAX_RETRIES; r++)
{
- case GNUNET_DB_STATUS_HARD_ERROR:
- case GNUNET_DB_STATUS_SOFT_ERROR:
- GNUNET_break (0);
- ret = TALER_MHD_reply_with_error (
- connection,
- MHD_HTTP_INTERNAL_SERVER_ERROR,
- TALER_EC_GENERIC_DB_FETCH_FAILED,
- "get_validation");
- return (MHD_NO == ret) ? GNUNET_SYSERR : GNUNET_NO;
- case GNUNET_DB_STATUS_SUCCESS_NO_RESULTS:
- GNUNET_break (0);
- ret = TALER_MHD_reply_with_error (
- connection,
- MHD_HTTP_NOT_FOUND,
- TALER_EC_CHALLENGER_GENERIC_VALIDATION_UNKNOWN,
- NULL);
- return (MHD_NO == ret) ? GNUNET_SYSERR : GNUNET_NO;
- case GNUNET_DB_STATUS_SUCCESS_ONE_RESULT:
+ qs = CHALLENGERDB_get_validation (CH_context,
+ nonce,
+ &client_secret,
+ &address,
+ &client_scope,
+ &client_state,
+ &client_redirect_uri);
+ switch (qs)
+ {
+ case GNUNET_DB_STATUS_HARD_ERROR:
+ GNUNET_break (0);
+ ret = TALER_MHD_reply_with_error (
+ connection,
+ MHD_HTTP_INTERNAL_SERVER_ERROR,
+ TALER_EC_GENERIC_DB_FETCH_FAILED,
+ "get_validation");
+ return (MHD_NO == ret) ? GNUNET_SYSERR : GNUNET_NO;
+ case GNUNET_DB_STATUS_SOFT_ERROR:
+ if (r < MAX_RETRIES - 1)
+ continue;
+ GNUNET_break (0);
+ ret = TALER_MHD_reply_with_error (
+ connection,
+ MHD_HTTP_INTERNAL_SERVER_ERROR,
+ TALER_EC_GENERIC_DB_FETCH_FAILED,
+ "get_validation");
+ return (MHD_NO == ret) ? GNUNET_SYSERR : GNUNET_NO;
+ case GNUNET_DB_STATUS_SUCCESS_NO_RESULTS:
+ GNUNET_break (0);
+ ret = TALER_MHD_reply_with_error (
+ connection,
+ MHD_HTTP_NOT_FOUND,
+ TALER_EC_CHALLENGER_GENERIC_VALIDATION_UNKNOWN,
+ NULL);
+ return (MHD_NO == ret) ? GNUNET_SYSERR : GNUNET_NO;
+ case GNUNET_DB_STATUS_SUCCESS_ONE_RESULT:
+ break;
+ }
break;
- }
+ } /* for MAX_RETRIES */
{
char *code;
char sep;
diff --git a/src/challenger/challenger-httpd_setup.c b/src/challenger/challenger-httpd_setup.c
@@ -32,6 +32,11 @@
*/
#define DEBUG 0
+/**
+ * Maximum number of retries for the database interaction.
+ */
+#define MAX_RETRIES 3
+
struct SetupContext
{
@@ -179,33 +184,48 @@ CH_handler_setup (struct CH_HandlerContext *hc,
sizeof (nonce));
/* Authenticating the client, charging it for the validation and
creating the validation row all happen in this one statement, so
- the validation counter only moves if the setup really succeeded. */
- qs = CHALLENGERDB_do_insert_validation (CH_context,
- client_id,
- client_secret,
- &nonce,
- expiration_time,
- sc->root);
- switch (qs)
+ the validation counter only moves if the setup really succeeded.
+ As the session is SERIALIZABLE and every /setup of a given client
+ updates the same 'validation_counter' row, serialization failures
+ are routine here and must be retried instead of reported. */
+ for (unsigned int r = 0; r<MAX_RETRIES; r++)
{
- case GNUNET_DB_STATUS_HARD_ERROR:
- case GNUNET_DB_STATUS_SOFT_ERROR:
- GNUNET_break (0);
- return TALER_MHD_reply_with_error (
- hc->connection,
- MHD_HTTP_INTERNAL_SERVER_ERROR,
- TALER_EC_GENERIC_DB_STORE_FAILED,
- "do_insert_validation");
- case GNUNET_DB_STATUS_SUCCESS_NO_RESULTS:
- /* Nothing was inserted, so the client did not authenticate. */
- return TALER_MHD_reply_with_error (
- hc->connection,
- MHD_HTTP_NOT_FOUND,
- TALER_EC_CHALLENGER_GENERIC_CLIENT_UNKNOWN,
- NULL);
- case GNUNET_DB_STATUS_SUCCESS_ONE_RESULT:
+ qs = CHALLENGERDB_do_insert_validation (CH_context,
+ client_id,
+ client_secret,
+ &nonce,
+ expiration_time,
+ sc->root);
+ switch (qs)
+ {
+ case GNUNET_DB_STATUS_HARD_ERROR:
+ GNUNET_break (0);
+ return TALER_MHD_reply_with_error (
+ hc->connection,
+ MHD_HTTP_INTERNAL_SERVER_ERROR,
+ TALER_EC_GENERIC_DB_STORE_FAILED,
+ "do_insert_validation");
+ case GNUNET_DB_STATUS_SOFT_ERROR:
+ if (r < MAX_RETRIES - 1)
+ continue;
+ GNUNET_break (0);
+ return TALER_MHD_reply_with_error (
+ hc->connection,
+ MHD_HTTP_INTERNAL_SERVER_ERROR,
+ TALER_EC_GENERIC_DB_STORE_FAILED,
+ "do_insert_validation");
+ case GNUNET_DB_STATUS_SUCCESS_NO_RESULTS:
+ /* Nothing was inserted, so the client did not authenticate. */
+ return TALER_MHD_reply_with_error (
+ hc->connection,
+ MHD_HTTP_NOT_FOUND,
+ TALER_EC_CHALLENGER_GENERIC_CLIENT_UNKNOWN,
+ NULL);
+ case GNUNET_DB_STATUS_SUCCESS_ONE_RESULT:
+ break;
+ }
break;
- }
+ } /* for MAX_RETRIES */
{
char *nstr;
diff --git a/src/challenger/meson.build b/src/challenger/meson.build
@@ -13,6 +13,7 @@ check_SCRIPTS = [
'test-challenger-revisit',
'test-challenger-badutf8',
'test-challenger-pinlimit',
+ 'test-challenger-db-retry',
]
test_helper_cat = configure_file(input: 'cat.sh', output: 'cat.sh', copy: true)
diff --git a/src/challenger/test-challenger-db-retry.sh b/src/challenger/test-challenger-db-retry.sh
@@ -0,0 +1,141 @@
+#!/usr/bin/env bash
+# This file is in the public domain.
+#
+# Regression test for finding 5: '/setup' must survive a serialization
+# failure instead of answering 500.
+#
+# The database session is SERIALIZABLE (see challengerdb/pg.c), so every
+# single-statement query is its own serializable transaction. The first
+# statement of 'do_insert_validation' is
+# UPDATE clients SET validation_counter=validation_counter+1 ...
+# which every /setup of a given client performs on the *same* row. Any
+# concurrent writer of that row therefore makes /setup fail with SQLSTATE
+# 40001 -> GNUNET_DB_STATUS_SOFT_ERROR, which the handler must retry.
+#
+# We provoke exactly that, deterministically: a psql session holds an open
+# transaction that has already updated the counter row, /setup is issued
+# (and blocks on the row lock), and the holder then commits. PostgreSQL
+# then aborts the blocked statement with 40001. With the retry loop the
+# request succeeds on the next attempt; without it the user gets a 500.
+
+set -eu
+
+# Exit, with status code "skip" (no 'real' failure)
+function exit_skip() {
+ echo " SKIP: $1"
+ exit 77
+}
+
+# Exit, with error message (hard failure)
+function exit_fail() {
+ echo " FAIL: $@"
+ exit 1
+}
+
+# Cleanup to run whenever we exit
+function cleanup()
+{
+ exec 3>&- 2> /dev/null || true
+ for n in $(jobs -p)
+ do
+ kill $n 2> /dev/null || true
+ done
+ rm -f "$LAST_RESPONSE" "$BLOCKER_FIFO"
+ wait
+}
+
+LAST_RESPONSE=$(mktemp responseXXXXXX.log)
+BLOCKER_FIFO=$(mktemp -u blockerXXXXXX.fifo)
+
+# Install cleanup handler (except for kill -9)
+trap cleanup EXIT
+
+export PATH="$PATH:."
+
+echo -n "Testing for jq"
+jq -h > /dev/null || exit_skip "jq required"
+echo " FOUND"
+echo -n "Testing for curl"
+curl -h > /dev/null || exit_skip "curl required"
+echo " FOUND"
+echo -n "Testing for wget"
+wget -h > /dev/null || exit_skip "wget required"
+echo " FOUND"
+echo -n "Testing for psql"
+psql --version > /dev/null || exit_skip "psql required"
+echo " FOUND"
+echo -n "Testing for challenger-httpd ..."
+challenger-httpd -h > /dev/null || exit_skip "challenger-httpd required"
+echo " FOUND"
+
+CONF="test-challenger.conf"
+BURL="http://localhost:9967"
+REDIRECT_URI="http://client.example.com/"
+
+DB_URI=$(challenger-config -c "${CONF}" -s challengerdb-postgres -o CONFIG)
+
+echo -n "Initialize challenger database ..."
+challenger-dbinit -r -c "${CONF}" &> dbinit.log
+echo " OK"
+
+echo -n "Add challenger client ..."
+CLIENT_SECRET="secret-token:secret"
+challenger-admin -c "${CONF}" -a "${CLIENT_SECRET}" "${REDIRECT_URI}" &> admin.log
+echo " OK"
+# We just reset the DB, thus the client ID must be 1 here:
+CLIENT_ID=1
+
+echo -n "Start challenger-httpd ..."
+challenger-httpd -L INFO -c "${CONF}" &> httpd.log &
+
+# Wait for challenger to be available
+for n in $(seq 1 50)
+do
+ echo -n "."
+ sleep 0.2
+ OK=0
+ wget --tries=1 --timeout=1 "${BURL}/config" -o /dev/null -O /dev/null >/dev/null || continue
+ OK=1
+ break
+done
+if [ 1 != $OK ]
+then
+ exit_skip "Failed to launch challenger service"
+fi
+echo " OK"
+
+echo -n "Locking the client's validation counter ..."
+mkfifo "${BLOCKER_FIFO}"
+psql "${DB_URI}" -q -f "${BLOCKER_FIFO}" &> blocker.log &
+exec 3>"${BLOCKER_FIFO}"
+echo "BEGIN; UPDATE challenger.clients SET validation_counter=validation_counter+1 WHERE client_serial_id=${CLIENT_ID};" >&3
+sleep 1
+echo " OK"
+
+echo -n "Setup new validation process against the lock ..."
+curl "${BURL}/setup/${CLIENT_ID}" \
+ -H "Authorization: Bearer ${CLIENT_SECRET}" \
+ -d '' \
+ -w "%{http_code}" -s -o $LAST_RESPONSE > status.txt &
+CURL_PID=$!
+# Give the request time to reach the UPDATE and block on the row lock,
+# then release it; the blocked statement now fails with 40001.
+sleep 2
+echo "COMMIT;" >&3
+exec 3>&-
+wait $CURL_PID
+STATUS=$(cat status.txt)
+rm -f status.txt
+
+if [ "$STATUS" != "200" ]
+then
+ exit_fail "Expected 200 OK after retrying the serialization failure. Got: $STATUS" $(cat $LAST_RESPONSE)
+fi
+NONCE=$(jq -r .nonce < "$LAST_RESPONSE")
+if [ "x$NONCE" = "xnull" ]
+then
+ exit_fail "Expected a nonce in the /setup response"
+fi
+echo " OK"
+
+exit 0
