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

CHANGELOG-INTERNAL.md

@@ -42,6 +42,35 @@ a `### BREAKING` section placed FIRST in that version, and each such line is pre
 - Help text and the flag tables in both READMEs now state that the flag is valid on its own.
 - Version bump deliberately NOT taken (convention 34): the owner closes it in `VERSION.txt`.
 
+### Fixed: a dropped packet no longer revives the NAT mapping it was dropped for (audit F1)
+
+`BeanCore.decide()` step 3 read and wrote the activity stamp in one `_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 the flow as active, and the mapping was back: the direction
+lost exactly one packet per `nat_timeout_s` and carried traffic in between, with nothing outbound
+involved. Measured before the fix (timeout 5 s, one outbound at t=0, inbound only afterwards):
+`t=10 DROP, t=11 pass, t=12 pass, t=13 pass, t=20 DROP`. The impairment exists to test whether an
+application sends keep-alives, and in that shape the test could not fail.
+
+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 at 200k
+iterations, difference below the noise floor. `_FlowTable.touch()` had exactly one caller and is
+removed with it.
+
+How long the blackhole holds is bounded by the flow table, and the comment in `core.py` now says
+so with numbers instead of a general claim: the drop path returns above the `_prune()` call, so it
+never rotates anything itself. Measured with the same setup - this flow alone: still blackholed at
+t=200; one other flow driving `_prune`: reopened at t=30, the first rotation. That is the table's
+documented safe direction (it can lose an impairment, never invent one) and is a different thing
+from the resurrection above, where the dropped packet did the reopening itself.
+
+New test: `tests/test_core.py::test_a_dropped_packet_does_not_revive_an_expired_nat_mapping` -
+asserts every inbound packet after expiry is dropped (not just the first) and that an OUTBOUND
+packet is what brings the mapping back. Verified by mutation, not assumed: restoring the
+write-before-verdict order turns it red with `drops=[True, False, False, False]`, the exact
+symptom above. The two existing NAT tests (`test_nat_expiry`, `test_nat_outbound_refreshes`) pass
+unchanged - neither of them pinned the buggy behaviour, which is why it survived.
+
 ### Performance: PortTable.refresh collects the socket table outside its lock
 
 The capture thread takes `PortTable._lock` too - `name_of(cheap=True)` -> `info()` in

CHANGELOG.md

@@ -36,6 +36,17 @@ The format follows [Keep a Changelog](https://keepachangelog.com/); versions fol
 
 ### Fixed
 
+- **"NAT mapping expiry" now really cuts the incoming direction, instead of losing one packet
+  every few seconds.** This impairment is there to answer one question: does the application
+  notice its mapping is gone and send something to re-open it? It could not answer that. The
+  packet the tool rejected for "the mapping has expired" was itself counted as activity on that
+  connection, so the mapping came back on the spot and traffic flowed again for another whole
+  timeout - then one more packet was dropped, and so on. With `--nat-timeout 5` an app that never
+  sent a keep-alive lost about one packet every five seconds and otherwise carried on working, so
+  it passed a test it should have failed. Now incoming traffic stays cut until the application
+  actually sends something outbound, which is what re-opens the mapping on a real NAT. Nothing
+  changes when you leave the setting at 0 (off, the default).
+
 - **Short-lived connections now show which program they belong to.** The Connections table works
   out the owning program by asking Windows which application holds each socket, and that answer
   used to come from a list refreshed a few times a second. A connection that opened and finished

beantester/core.py

@@ -105,13 +105,6 @@ def set(self, key, value):
         self._new[key] = value
         self._enforce_ceiling()
 
-    def touch(self, key, value):
-        """Read the previous value and write the new one in a single lookup pass."""
-        previous = self.get(key)
-        self._new[key] = value
-        self._enforce_ceiling()
-        return previous
-
     def _rotate(self):
         """Retire a generation. Truly O(1): nothing is freed on this thread."""
         if self._old:
@@ -479,11 +472,42 @@ def decide(self, size, is_outbound, local_port, now, rng,
             # NAT check is its only reader, and NAT is off by default. So the common
             # configuration paid for a table it never looked at: 50 000 packets left
             # 50 000 entries behind, for nothing.
+            #
+            # A DROPPED packet must not refresh the mapping. This used to be a
+            # single `touch()` (read the old stamp, write the new one), so the very
+            # packet being dropped for "your mapping is gone" wrote the stamp that
+            # brought it back: after the first drop, inbound traffic flowed again for
+            # a whole further timeout window with nothing outbound in between.
+            # Measured before the split (timeout 5 s, one outbound at t=0, inbound
+            # only): drop at t=10, then PASS at t=11, t=12, t=13, drop at t=20 - i.e.
+            # one lost packet per timeout instead of a direction that stays shut
+            # until the application sends something. That "does the app send its
+            # keep-alives" test is the entire point of this impairment, and it could
+            # not fail.
+            #
+            # Split into get + set so the write is skipped on the drop path. Same
+            # cost: `touch` was a get plus this same write (measured 160 ns/op both
+            # ways, difference below the noise floor at 200k iterations).
+            #
+            # Bounded honesty: the stamp lives in a _FlowTable, which retires a
+            # generation on age (FLOW_ROTATE_S) and on size, and a retired record
+            # reads back as "never seen" - so the direction CAN reopen with no
+            # outbound packet at all. How long the blackhole really holds turns on
+            # whether other flows are moving, because the drop path returns above the
+            # _prune() call and so never rotates anything itself. Measured (timeout
+            # 5 s, drops from t=10): with this flow alone the blackhole was still
+            # holding at t=200; with one other flow driving _prune it ended at t=30,
+            # the first rotation. So it lasts at least one rotation window, and holds
+            # indefinitely only when nothing else is on the wire. That is the table's
+            # documented safe direction (it can lose an impairment, never invent one)
+            # and it is NOT the resurrection above - there the dropped packet did the
+            # reopening itself, once per timeout, for as long as traffic kept coming.
             if self.nat_timeout_s > 0 and key is not None:
-                last = self._flow_last.touch(key, now)
+                last = self._flow_last.get(key)
                 expired = last is not None and (now - last) > self.nat_timeout_s
                 if expired and not is_outbound:
                     return Decision(True, False, [], "nat")
+                self._flow_last.set(key, now)
 
             # Rotate the bounded tables. O(1) (see _FlowTable) and throttled, so it
             # is safe to call from the hot path - which is the point: the tables must

tests/test_core.py

@@ -409,6 +409,32 @@ def test_nat_outbound_refreshes():
     check("NAT: outbound refreshes the mapping (inbound passes)", inn.drop is False)
 
 
+def test_a_dropped_packet_does_not_revive_an_expired_nat_mapping():
+    """An expired mapping stays expired until the application sends something.
+
+    The drop used to write the activity stamp itself (one `touch()` did the read
+    and the write), so the packet rejected for "the mapping is gone" reopened it:
+    the direction lost exactly ONE packet per timeout and then carried traffic
+    again, with nothing outbound in between. A keep-alive test that cannot fail is
+    worse than no test, because it reads as a pass.
+    """
+    core = BeanCore()
+    core.set_nat(5.0)
+    rng = random.Random(1)
+    kw = dict(remote_ip="1.2.3.4", remote_port=443, is_tcp=True)
+    core.decide(100, True, 5000, 0.0, rng, **kw)                  # mapping created
+    # inbound only from here on - nothing may reopen the mapping
+    verdicts = [core.decide(100, False, 5000, t, rng, **kw).drop
+                for t in (10.0, 11.0, 12.0, 13.0)]
+    check("NAT: every inbound packet after expiry is dropped, not just the first",
+          all(verdicts), f"(drops={verdicts})")
+    # ...and an OUTBOUND packet is what brings it back
+    out = core.decide(100, True, 5000, 14.0, rng, **kw)
+    back = core.decide(100, False, 5000, 14.5, rng, **kw)
+    check("NAT: outbound reopens the mapping", out.drop is False)
+    check("NAT: inbound flows again once the app has sent", back.drop is False)
+
+
 def test_reset_buckets_clears_flow_state():
     core = BeanCore()
     core.set_rst(100, 30.0)                  # long cooldown to make state persistent