Repository navigation
fix(wal): reject non-string keys and values on the write path - #345
adiraj66132 wants to merge 1 commit into
Conversation
danieldeer#344 added the check only on replay, so a non-string key or value could still be written. The next replay called that line corrupt and truncated the log there, dropping every valid write after it. Validate in SetEntry/DeleteEntry instead, before anything is written or applied, so insert/delete raise TypeError and leave memory and the log untouched.
|
Im not at my desk rn so I can't make sure if that true or not. |
|
I closed it because I wasn't sure whether the changes introduced any meaningful improvement over the current state of the project. However, my fix addresses a specific issue in the write path that the previous PR left unresolved: invalid entries can still be written to the WAL, causing replay to truncate the log and silently discard subsequent valid writes. I added validation before modifying memory or writing to the WAL, along with regression tests to verify that valid writes remain intact after a rejected write and restart. I haven't received a review yet, so I'd appreciate it if you could take a look when you're back at your desk. |
mattsva
left a comment
mattsva
left a comment
There was a problem hiding this comment.
Nit (optional): the key check is repeated in SetEntry and DeleteEntry. Fine for two small classes, but a shared helper would avoid drift if more entry types are added.
The rest would look good.
Why dont you see a use for that? I must missed those cases in the original PR, but that just means that I am stupid, not that this is wrong.
LGTM
| ------ | ||
| TypeError | ||
| If `key` or `value` is not a string. | ||
| ServiceUnavailableError |
| raise TypeError("value must be a string") | ||
|
|
||
| return SetEntry(key=key, value=value) | ||
| return SetEntry(key=data["key"], value=data["value"]) |
There was a problem hiding this comment.
Thats a lot simpler, I do like it
| key: str | ||
| value: str | ||
|
|
||
| def __post_init__(self) -> None: |
| ] | ||
|
|
||
|
|
||
| def test_entries_reject_non_string_key_or_value(): |
mattsva
left a comment
mattsva
left a comment
There was a problem hiding this comment.
Do you want it to be merged?
|
Yes, I'd like it to be merged. Thanks for reviewing it and for the feedback! I'll address the optional suggestion about sharing the key validation logic between |
The bug
#344 made the WAL reader reject entries whose key or value is not a string. The writer was left alone, so this still goes through:
On the next start, replay reaches the first line, calls it corrupt, and truncates the file right there. That deletes the bad entry and
keepwith it, and both writes had already returned success. One bad write silently drops every write after it, permanently, because the truncation happens on disk.I hit this while reviewing #344 on the current main, and confirmed it with a small script: three inserts, restart, empty dict, WAL truncated to 0 bytes.
The fix
Check where entries are built, not where they are parsed.
SetEntry/DeleteEntryvalidate in__post_init__and raiseTypeError.from_dictdrops its own copies of the check: it gets the sameTypeErrorfrom the constructor, so the check lives in one place instead of three.insert/deletenow raiseTypeErrorbefore touching memory or the log, the same all-or-nothing behaviour they already have when the append fails.TypeErroris caught, the log is truncated at that entry.Rejecting only on replay can never fix a bad line that is already on disk. The write side is where the fix belongs.
Tests
test_rejected_write_leaves_the_wal_healthy: a rejected write changes neither memory nor the WAL, and a write after it survives a restart. This one fails on main.test_entries_reject_non_string_key_or_value, plus adeletecase in the existing malformed-entry replay test.pytest: 190 passed (was 187).ty,ruff check,ruff format --checkandscripts/docs-reference.py --checkare clean.