diff --git a/modules/web_viewer/app.py b/modules/web_viewer/app.py index 75f14be..2ee9d01 100644 --- a/modules/web_viewer/app.py +++ b/modules/web_viewer/app.py @@ -4045,6 +4045,15 @@ class BotDataViewer: return jsonify({'success': False, 'error': described.get('error')}), 400 existing = _existing_schedules() + + # Checked here rather than in the route so it shares this snapshot and the + # surrounding lock. + if replacing is not None and replacing not in existing: + return jsonify({ + 'success': False, + 'error': f"No scheduled message for '{replacing}'", + }), 404 + # Schedules are INI keys, so two entries cannot share one. Renaming onto # another entry's key would silently overwrite it. if schedule in existing and schedule != replacing: @@ -4114,8 +4123,9 @@ class BotDataViewer: original = (data.get('original_schedule') or '').strip() if not original: return jsonify({'success': False, 'error': 'original_schedule is required'}), 400 - if original not in _existing_schedules(): - return jsonify({'success': False, 'error': f"No scheduled message for '{original}'"}), 404 + # Existence is verified inside the lock, against the same snapshot the + # duplicate check uses: a concurrent delete between an outside check and + # the write would otherwise resurrect the entry as a new one. return _save_scheduled_message(data, replacing=original) except Exception as e: self.logger.error(f"Error updating scheduled message: {e}") diff --git a/tests/test_scheduled_messages_api.py b/tests/test_scheduled_messages_api.py index 0fa907d..db48735 100644 --- a/tests/test_scheduled_messages_api.py +++ b/tests/test_scheduled_messages_api.py @@ -204,3 +204,44 @@ class TestRoundTrip: raw = parser.get("Scheduled_Messages", "15 9 * * mon,fri") channel, message, scope = parse_scheduled_message_value(raw) assert (channel, message, scope) == ("Public", "Standup at 9:15", "#sea") + + +class TestWriteSerialisation: + """Duplicate/existence checks must share the write's critical section, or a + concurrent request can slip between the check and the write.""" + + def test_concurrent_creates_of_one_schedule_yield_exactly_one_success(self, viewer): + import threading + + results = [] + barrier = threading.Barrier(2) + + def create(body): + client = viewer.app.test_client() + barrier.wait() + resp = _post(client, "/api/scheduled-messages", body) + results.append(resp.status_code) + + threads = [ + threading.Thread(target=create, args=({ + "schedule": "45 7 * * *", "channel": "Public", "message": f"msg {i}", + },)) for i in range(2) + ] + for t in threads: + t.start() + for t in threads: + t.join() + + assert sorted(results) == [200, 409], results + # And exactly one entry landed, not one silently replacing the other. + assert _config_text(viewer).count("45 7 * * * =") == 1 + + def test_update_of_a_removed_entry_is_a_404_not_a_resurrection(self, viewer): + client = _client(viewer) + _post(client, "/api/scheduled-messages", {"schedule": "0 8 * * *"}, method="delete") + resp = _post(client, "/api/scheduled-messages", { + "original_schedule": "0 8 * * *", "schedule": "0 9 * * *", + "channel": "Public", "message": "back from the dead", + }, method="put") + assert resp.status_code == 404 + assert "back from the dead" not in _config_text(viewer)