Forum teuk.org

🧹 *Scourgify DBI!* — DBCommands Cursor Cleanup and Statement Handle Hardening

in Mediabot · started by TeuK · 4w ago

TeuK · 4w ago

This Mediabot v3 pass was a focused reliability cleanup in Mediabot/DBCommands.pm.

The goal was not to add features.

The goal was to remove a quiet class of long-running bot problems:

DBI statement handles not closed on error paths
prepare()/execute() paths that could crash or leak cursors
admin/debug commands that could fail badly when MariaDB is unhealthy

No schema change.


Why this mattered

Most of these bugs do not show up when the database is healthy.

They show up when something goes wrong:

MariaDB temporarily unavailable
database handle stale or broken
table missing during migration/debug
SQL prepare succeeds but execute fails
SQL prepare fails and returns undef

In those cases, old code could do things like:

$sth->execute(...)
$sth->finish

without always proving that $sth existed.

Or it could correctly guard the execute call:

unless ($sth && $sth->execute(...)) {
    return;
}

but forget to close $sth when prepare() succeeded and execute() failed.

That second case is subtle: it does not always crash, but it can leak DBI cursors over time.

For a long-running IRC bot, that is exactly the kind of boring bug worth killing.


Main target: Mediabot/DBCommands.pm

This pass focused on DBCommands.pm, especially public-command database helpers and admin commands.

The audited areas included:

timers
responders
SQL-backed public commands
modcmd
chowncmd
showcmd
mvcmd
countcmd
topcmd
lastcmd
searchcmd
owncmd
holdcmd
addcatcmd
ignores
lastcom
yomomma

Responder path hardened

The responder path is important because it can run from normal public channel traffic.

Several issues were fixed around:

checkResponder()
doResponder()

checkResponder()

The successful match path now closes the cursor before returning immediately.

The fallback path also avoids calling finish on an undefined statement handle:

$sth->finish if $sth;

doResponder()

Both the responder lookup and the hit counter update now guard prepare() failure before execute().

The hit update now also closes its statement handle cleanly.

This matters because responders can be hit frequently on active channels.


SQL public command dispatcher hardened

mbDbCommand() now guards both:

SELECT command/action
UPDATE hits

This prevents a normal SQL-backed public command from crashing the dispatcher when the database is unstable.

The command can now fail cleanly instead of trying to call execute() on an undefined statement handle.


Command administration hardened

Several command-management helpers were cleaned up:

modcmd
chowncmd
showcmd
mvcmd

The recurring fix was:

unless ($sth && $sth->execute(...)) {
    ...
    $sth->finish if $sth;
    return;
}

This was applied where needed so failed execute() paths close the cursor before returning.

modcmd

Both legacy and context-based paths were checked.

The update statement now guards failed prepare() and closes handles on error.

chowncmd

The command lookup, target user lookup, and owner update now close statement handles on execute failure.

showcmd

The command display query now follows the same safe error path.

mvcmd

The rename flow was hardened:

check that new command name does not already exist
load old command
update command name

All three DB operations now close statement handles correctly on error.


Diagnostic command helpers hardened

The following database-backed diagnostic/admin commands were cleaned up:

countcmd
topcmd
lastcmd
searchcmd
owncmd
holdcmd

These commands are often used exactly when something feels wrong, so they should not make a database problem worse.

They now fail with controlled logging/notices and close statement handles on error.


Category, ignore, and responder admin paths

The following areas were also cleaned up:

addcatcmd
addResponder_ctx
delResponder_ctx
IgnoresList_ctx
addIgnore_ctx
delIgnore_ctx

These are not high-volume paths, but they are long-lived administration paths.

The same DBI rule now applies consistently:

if prepare succeeded and execute failed, close the handle before return

lastcom and yomomma cleaned up

lastcom

The ACTIONS_LOG query now closes its statement handle if execute fails.

yomomma

All three query paths were hardened:

specific joke by ID
COUNT(*) for random mode
random OFFSET lookup

Each branch now closes the statement handle if execute fails.


Final scan result

After all patches, a global scan was run against DBCommands.pm.

The most important result:

===== Scan 2: guarded execute blocks with return but no finish in block =====

was empty.

That means no remaining block was found in the risky form:

unless ($sth && $sth->execute(...)) {
    ...
    return;
}

without a corresponding:

$sth->finish if $sth;

before returning.


False positives confirmed

The remaining direct ->execute() calls reported by the scan were manually inspected.

They all use the safe split pattern:

unless ($sth) {
    ...
    return;
}

unless ($sth->execute(...)) {
    ...
    $sth->finish;
    return;
}

So $sth is guaranteed to exist before calling execute().

Confirmed safe areas included:

onStartTimers()
mbAddTimer_ctx()
mbRemTimer_ctx()
mbTimers_ctx()
getCommandCategory()
mbPopCommand_ctx()

Validation

Minimum validation:

perl -I. -c Mediabot/DBCommands.pm
perl -c mediabot.pl

Recommended wider validation:

find Mediabot -name '*.pm' -print -exec perl -I. -c {} \;
perl -c mediabot.pl

Result

This pass leaves DBCommands.pm much cleaner for DBI error handling:

no known undef->execute path in the audited file
no known guarded execute + early return without finish
statement handles are closed on execute failure
responders and SQL-backed commands are safer under DB trouble
admin diagnostic commands fail more cleanly
long-running cursor leak risk is reduced

No schema change.


Spell of the day

Scourgify DBI!

No new magic features.

Just fewer dirty cursors, fewer fragile DB paths, and a bot that should survive MariaDB hiccups with a little more dignity.

Mischief cleaned. 🧹🧙‍♂️

You must be logged in to reply.