[BUG] SQLite/APSW retry exhaustion reports abandoned writes as successful
Nobody has claimed this yet.
- Dominant language
- Python
- Stars
- 5.6k
- Forks
- 1.2k
- Avg merge
- 1d 21h
- Merged PRs (30d)
- 16
Description
Description
In the APSW/SQLite path, submit_logs_to_db() and submit_temp_logs_to_db() return True after all database is locked retries are exhausted, even though the INSERT was rolled back and no COMMIT occurred.
Both functions document their return value as:
True if success otherwise False
The corresponding exhaustion path therefore appears to violate the function contract.
Current behavior
In nettacker/database/db.py, both functions handle apsw.BusyError("database is locked") by rolling back and retrying.
After Config.settings.max_retries attempts are exhausted, execution falls through to:
logger.warn(messages("database_retries_exhausted"))
return True
At this point, the INSERT has never reached send_submit_query() and there is no committed row.
This differs from the other failure paths, which return False.
Why this matters
submit_logs_to_db() writes to scan_events, including the target, module, port, and condition results associated with scan findings.
submit_temp_logs_to_db() writes intermediate state to temp_events, which is used by multi-step module processing.
The production calls in nettacker/core/lib/base.py currently discard the return value from both functions. process_conditions() then continues and reports the event as successful regardless of whether the database write actually succeeded.
This means an exhausted SQLite write can currently be:
- Rolled back
- Abandoned
- Reported as successful by the DB function
- Ignored by the caller
- Followed by the normal success path
Existing test coverage
There are already deterministic tests for the database is locked exhaustion path.
However, test_apsw_busy_error does not assert the return value, while test_temp_log_busy_error explicitly asserts that the result is truthy after exhaustion:
assert result # we're continuing operation hence it returns True
This appears to encode the current behavior rather than the documented True if success otherwise False contract.
Proposed fix
I would like to get maintainer feedback on the fix before implementing it.
One option would be to:
- Return
Falseafter retry exhaustion in both functions. - Update the affected test expectation.
- Add an explicit return-value assertion to the existing
test_apsw_busy_error.
This would make the return value consistent with the documented contract and the other failure paths, without changing the existing retry count, transaction handling, threading model, or database architecture.
I am intentionally not proposing a database queue or broader retry architecture at this stage.
Question for maintainers
Would returning False on database_retries_exhausted be the preferred contract, while retaining the current "continue the scan" behavior?
If so, I can work on a small regression fix and tests.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start in nettacker/database/db.py by tracing the APSW BusyError exhaustion paths in submit_logs_to_db() and submit_temp_logs_to_db(). Run test_apsw_busy_error and test_temp_log_busy_error, then update the exhaustion expectations and add the missing return-value assertion. Done means exhausted writes return False while retry behavior remains unchanged.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python, sqlite
- Domain
- backend, databases
- Issue type
- Bug
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 78/100