Skip to content

fix(core): keep an expired NAT mapping expired until the app sends - #51

Merged
donislawdev merged 1 commit into
masterfrom
fix/nat-expiry-stays-expired
Jul 27, 2026
Merged

fix(core): keep an expired NAT mapping expired until the app sends#51
donislawdev merged 1 commit into
masterfrom
fix/nat-expiry-stays-expired

Conversation

@donislawdev

Copy link
Copy Markdown
Owner

What and why

Found during an engineering audit of the engine (finding F1).

BeanCore.decide() step 3 read and wrote the flow activity stamp in a single
_FlowTable.touch(), so the write happened before the expiry verdict and applied to the drop
path too. The packet rejected with reason nat therefore stamped its own flow as active and
brought the mapping straight back.

Measured before the fix (timeout 5 s, one outbound packet at t=0, inbound only afterwards):

t= 0.0 out -> pass
t=10.0 in  -> DROP
t=11.0 in  -> pass     <- the dropped packet reopened the mapping
t=12.0 in  -> pass
t=13.0 in  -> pass
t=20.0 in  -> DROP

So the direction lost roughly one packet per timeout and otherwise kept working, with nothing
outbound involved. This impairment exists to answer "does the application notice its mapping is
gone and send something to re-open it" - and in that shape the test could not fail: an app with no
keep-alive passed it. After the fix every inbound packet stays dropped until an outbound packet
re-opens the mapping, which is what a real NAT does.

How

  • core.py: split into get() plus a set() placed after the verdict, so the drop path skips
    the write. Same cost - touch() was a get plus this same write. Measured 160 ns/op both ways
    at 200k iterations, difference below the noise floor, so the packet hot path does not regress.
  • core.py: _FlowTable.touch() had exactly one caller and is removed with it.
  • core.py: the comment now bounds the blackhole with measurements instead of a general claim.
    The drop path returns above the _prune() call, so it never rotates anything itself: with this
    flow alone the blackhole was still holding at t=200; with one other flow driving _prune it
    reopened at t=30, the first rotation. That is the flow table's documented safe direction (it
    can lose an impairment, never invent one) and is a separate matter from the resurrection fixed
    here.

For the reviewer

  • The new test was verified by mutation, not assumed. Restoring the old write-before-verdict
    order turns it red with drops=[True, False, False, False] - the exact symptom above.
  • Both existing NAT tests pass unchanged (test_nat_expiry, test_nat_outbound_refreshes).
    Neither of them pinned the buggy behaviour, which is why it survived; that gap is what the new
    test closes.
  • No behaviour change when nat_timeout is left at its default of 0 (off).
  • No public contract touched: no CLI flag, exit code, NDJSON field or on-disk format changes.

Checklist

  • python -m pytest tests passes locally (670 tests, 0 failures, elevated shell). python smoke_gui.py passes.
  • New behaviour has tests (tests/test_core.py::test_a_dropped_packet_does_not_revive_an_expired_nat_mapping).
  • UI text goes through i18n keys, with both lang/en.json and lang/pl.json updated - not applicable, no UI strings in this change.
  • User-facing changes noted in CHANGELOG.md; technical ones and new tests in CHANGELOG-INTERNAL.md, under [Unreleased].
  • Commits follow Conventional Commits (type(scope): summary).
  • No version bump - the owner closes a version via VERSION.txt.

🤖 Generated with Claude Code

Step 3 of the pipeline read and wrote the flow activity stamp in one
touch(), so the write landed BEFORE the expiry verdict and applied to the
drop path too: the packet rejected with reason "nat" stamped its own flow
as active and brought the mapping straight back. Measured (timeout 5 s,
one outbound at t=0, inbound only after): t=10 DROP, t=11/12/13 pass,
t=20 DROP - one lost packet per timeout instead of a direction that stays
shut. This impairment exists to test whether an application sends its
keep-alives, and in that shape the test could not fail.

- core.py: split into get() + a set() placed after the verdict, so the
  drop path skips the write. Same cost (touch was a get plus this same
  write): measured 160 ns/op both ways, difference below the noise floor.
- core.py: drop _FlowTable.touch(), which had exactly one caller.
- core.py: the comment now bounds the blackhole with measurements instead
  of a general claim - this flow alone still blackholed at t=200, one
  other flow driving _prune reopens it at t=30 (first rotation).
- tests: test_a_dropped_packet_does_not_revive_an_expired_nat_mapping,
  verified by mutation (old order -> drops=[True, False, False, False]).
- both changelogs (convention 39).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@donislawdev
donislawdev merged commit 8c4610d into master Jul 27, 2026
8 checks passed
@donislawdev
donislawdev deleted the fix/nat-expiry-stays-expired branch July 27, 2026 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant