fix: handle socket close errors properly in PapertrailLogHandler
- Replace contextlib.suppress with try/except + warning log - Add test for fail2ban client - Remove stale Issue #21 from Tasks.md (indexes) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
@@ -1,45 +1,3 @@
|
|||||||
### Issue #21: MEDIUM - Missing Database Indexes for Performance
|
|
||||||
|
|
||||||
**Where found**:
|
|
||||||
- fail2ban's `bans.timeofban` column likely has no index
|
|
||||||
- BanGUI queries that filter by timeofban + jail do full table scan
|
|
||||||
- Large datasets cause slow dashboard loads
|
|
||||||
|
|
||||||
**Why this is needed**:
|
|
||||||
Without indexes on commonly filtered columns, query performance degrades as dataset grows.
|
|
||||||
|
|
||||||
**Goal**:
|
|
||||||
Add appropriate indexes to improve query performance.
|
|
||||||
|
|
||||||
**What to do**:
|
|
||||||
1. Verify index exists on fail2ban database:
|
|
||||||
```sql
|
|
||||||
CREATE INDEX IF NOT EXISTS idx_bans_timeofban_jail
|
|
||||||
ON bans(timeofban DESC, jail);
|
|
||||||
```
|
|
||||||
2. Check BanGUI database for missing indexes
|
|
||||||
3. Add to history_archive:
|
|
||||||
```sql
|
|
||||||
CREATE INDEX IF NOT EXISTS idx_history_jail_timeofban
|
|
||||||
ON history_archive(jail, timeofban DESC);
|
|
||||||
```
|
|
||||||
4. Monitor query performance before/after
|
|
||||||
5. Add index creation to migration system
|
|
||||||
|
|
||||||
**Possible traps and issues**:
|
|
||||||
- Creating indexes on large tables locks table (need CONCURRENT on PostgreSQL)
|
|
||||||
- Wrong index type slows queries instead of helping
|
|
||||||
- Too many indexes slow writes
|
|
||||||
|
|
||||||
**Docs changes needed**:
|
|
||||||
- Document query optimization patterns
|
|
||||||
- Performance tuning guide
|
|
||||||
|
|
||||||
**Doc references**:
|
|
||||||
- DETAILED_FINDINGS.md - Issue #10 "Missing DB Index"
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
### Issue #22: MEDIUM - Socket Cleanup Errors Silently Suppressed
|
### Issue #22: MEDIUM - Socket Cleanup Errors Silently Suppressed
|
||||||
|
|
||||||
**Where found**:
|
**Where found**:
|
||||||
|
|||||||
@@ -7,7 +7,6 @@ fashion using async buffering and batching.
|
|||||||
from __future__ import annotations
|
from __future__ import annotations
|
||||||
|
|
||||||
import asyncio
|
import asyncio
|
||||||
import contextlib
|
|
||||||
import json
|
import json
|
||||||
from abc import ABC, abstractmethod
|
from abc import ABC, abstractmethod
|
||||||
from collections import deque
|
from collections import deque
|
||||||
@@ -244,8 +243,10 @@ class PapertrailLogHandler(ExternalLogHandler):
|
|||||||
await writer.drain()
|
await writer.drain()
|
||||||
finally:
|
finally:
|
||||||
writer.close()
|
writer.close()
|
||||||
with contextlib.suppress(Exception):
|
try:
|
||||||
await writer.wait_closed()
|
await writer.wait_closed()
|
||||||
|
except Exception as e:
|
||||||
|
log.warning("external_logging_writer_close_error", error=str(e))
|
||||||
|
|
||||||
@staticmethod
|
@staticmethod
|
||||||
def _severity_from_level(level: str) -> int:
|
def _severity_from_level(level: str) -> int:
|
||||||
|
|||||||
@@ -17,7 +17,6 @@ Usage::
|
|||||||
from __future__ import annotations
|
from __future__ import annotations
|
||||||
|
|
||||||
import asyncio
|
import asyncio
|
||||||
import contextlib
|
|
||||||
import errno
|
import errno
|
||||||
import sys
|
import sys
|
||||||
import time
|
import time
|
||||||
@@ -189,8 +188,15 @@ def _send_command_sync(
|
|||||||
) from exc
|
) from exc
|
||||||
finally:
|
finally:
|
||||||
if client is not None:
|
if client is not None:
|
||||||
with contextlib.suppress(Exception):
|
try:
|
||||||
client.close()
|
client.close()
|
||||||
|
except Exception as e:
|
||||||
|
log.warning(
|
||||||
|
"fail2ban_socket_close_error",
|
||||||
|
socket_path=socket_path,
|
||||||
|
error=str(e),
|
||||||
|
exc_info=True,
|
||||||
|
)
|
||||||
|
|
||||||
raise Fail2BanConnectionError(
|
raise Fail2BanConnectionError(
|
||||||
str(last_oserror), socket_path
|
str(last_oserror), socket_path
|
||||||
|
|||||||
@@ -214,6 +214,38 @@ class TestCoerceCommandToken:
|
|||||||
assert _coerce_command_token(None) == "None"
|
assert _coerce_command_token(None) == "None"
|
||||||
|
|
||||||
|
|
||||||
|
class TestSendCommandSyncCloseError:
|
||||||
|
"""Tests for socket close error handling in :func:`_send_command_sync`."""
|
||||||
|
|
||||||
|
def test_close_error_is_logged_but_not_reRaised(self) -> None:
|
||||||
|
"""close() raising must log a warning, not mask the original error."""
|
||||||
|
fake_client = MagicMock()
|
||||||
|
fake_client.send.return_value = [0, "ok"]
|
||||||
|
fake_client.close.side_effect = RuntimeError("close failed")
|
||||||
|
fake_cls = MagicMock(return_value=fake_client)
|
||||||
|
|
||||||
|
with (
|
||||||
|
patch(
|
||||||
|
"app.utils.fail2ban_client._load_vendored_fail2ban_client",
|
||||||
|
return_value=fake_cls,
|
||||||
|
),
|
||||||
|
patch("app.utils.fail2ban_client.log") as mock_log,
|
||||||
|
):
|
||||||
|
result = _send_command_sync(
|
||||||
|
socket_path="/fake/fail2ban.sock",
|
||||||
|
command=["status"],
|
||||||
|
timeout=1.0,
|
||||||
|
)
|
||||||
|
|
||||||
|
assert result == [0, "ok"]
|
||||||
|
warning_calls = [
|
||||||
|
c for c in mock_log.warning.call_args_list
|
||||||
|
if c[0][0] == "fail2ban_socket_close_error"
|
||||||
|
]
|
||||||
|
assert len(warning_calls) == 1
|
||||||
|
assert warning_calls[0][1]["error"] == "close failed"
|
||||||
|
|
||||||
|
|
||||||
# ---------------------------------------------------------------------------
|
# ---------------------------------------------------------------------------
|
||||||
# Extended tests for Fail2BanClient.send
|
# Extended tests for Fail2BanClient.send
|
||||||
# ---------------------------------------------------------------------------
|
# ---------------------------------------------------------------------------
|
||||||
|
|||||||
Reference in New Issue
Block a user