screen: add guarded SYN cookie runtime path#1409
Conversation
|
@copilot review |
Agent-Logs-Url: https://github.com/psaab/xpf/sessions/07153275-84bc-4d85-9e90-bbb0cf25381c Co-authored-by: psaab <196946+psaab@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds guarded SYN-cookie runtime scaffolding to the userspace dataplane screen path (still capability-gated), carrying a syn_cookie flag from Go snapshots into Rust, adding a fail-closed cookie challenge verdict, and wiring session-miss ACK validation plus a bounded validated-client cache into the AF_XDP pipeline.
Changes:
- Propagate
syn_cookiescreen profile state from Go → JSON protocol → RustScreenProfile. - Extend Rust screen runtime with
SynCookieChallenge, session-miss ACK validation, and a single-use validated-client cache. - Wire AF_XDP session-miss path to consume valid cookie ACKs before attempting session creation; update tests/docs accordingly.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| userspace-dp/src/screen.rs | Adds SYN-cookie challenge/ACK-validation/cache plumbing and parses TCP seq/ack/MSS from frames. |
| userspace-dp/src/screen_tests.rs | Extends unit tests to cover the new cookie runtime slice behaviors. |
| userspace-dp/src/protocol.rs | Adds syn_cookie to Rust snapshot deserialization. |
| userspace-dp/src/afxdp/poll_stages.rs | Wires zone-id-aware screen checks and adds session-miss cookie ACK validation stage. |
| userspace-dp/src/afxdp/poll_descriptor.rs | Calls the new session-miss cookie ACK stage before continuing miss resolution. |
| userspace-dp/src/afxdp/forwarding_build.rs | Maps syn_cookie from config snapshot into runtime ScreenProfile. |
| pkg/dataplane/userspace/snapshot.go | Sets SYNCookie on screen snapshots when global mode is syn-cookie. |
| pkg/dataplane/userspace/protocol.go | Adds syn_cookie JSON field to Go snapshot struct. |
| pkg/dataplane/userspace/manager_test.go | Adds a Go test asserting snapshot propagation of SYNCookie. |
| docs/userspace-dataplane-gaps.md | Updates the documented state of the SYN-cookie gap. |
| docs/pr/1373-retire-ebpf-dataplane/plan-1374-syn-cookies.md | Records current slice status and expands required test list. |
Comments suppressed due to low confidence (3)
userspace-dp/src/screen.rs:835
- SYN-flood counting / cookie-challenge selection should ignore non-first fragments (tcp_flags is unreliable on subsequent fragments, as noted earlier in the file). Without the same
!is_fragment || is_first_fragmentguard used for other TCP flag checks, a later fragment can spuriously incrementsyn_countersand trigger drops/challenges.
This issue also appears in the following locations of the same file:
- line 843
- line 942
if counter.increment(now_secs, profile.udp_flood_threshold) {
return ScreenVerdict::Drop("udp-flood");
}
}
// SYN flood: count TCP SYN (without ACK) per zone
userspace-dp/src/screen.rs:850
syn_cookie_active_until_secsis set before confirming a SYN-cookie codec is available. This means one threshold crossing withsyn_cookie=truebut no published master key activates cookie mode for the zone and will cause subsequent session-miss ACKs to be treated as Invalid (dropped) for up to EPOCH_SECS even though no cookies could be minted. Consider only activating cookie mode aftersyn_cookie_codecis Some, or making ACK validation NotApplicable when the codec is absent.
now_secs,
);
if !syn_cookie_validated {
let counter = self.syn_counters.entry(zone.to_string()).or_default();
if counter.increment(now_secs, profile.syn_flood_threshold) {
if profile.syn_cookie {
self.syn_cookie_active_until_secs.insert(
zone.to_string(),
userspace-dp/src/screen.rs:948
- ACK validation should also exclude non-first fragments for the same reason as the TCP stateless checks: fragments after the first don't reliably carry TCP header fields. As written, a subsequent fragment during an active cookie window could be misclassified as an ACK and get dropped as Invalid (or attempt validation with zeroed tcp_ack).
now_secs: u64,
) -> SynCookieAckVerdict {
let Some(profile) = self.profiles.get(zone) else {
return SynCookieAckVerdict::NotApplicable;
};
if !profile.syn_cookie || profile.syn_flood_threshold == 0 || pkt.protocol != PROTO_TCP {
return SynCookieAckVerdict::NotApplicable;
| @@ -565,9 +679,20 @@ impl ScreenState { | |||
| self.icmp_counters.retain(|k, _| profiles.contains_key(k)); | |||
| _ => break, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if protocol == PROTO_TCP | ||
| && (!info.is_fragment || info.is_first_fragment) | ||
| && let Some(tcp_start) = tcp_offset | ||
| && tcp_start + 20 <= frame.len() | ||
| { | ||
| let tcp = &frame[tcp_start..]; | ||
| info.tcp_seq = u32::from_be_bytes([tcp[4], tcp[5], tcp[6], tcp[7]]); | ||
| info.tcp_ack = u32::from_be_bytes([tcp[8], tcp[9], tcp[10], tcp[11]]); | ||
| let data_offset = ((tcp[12] >> 4) as usize) * 4; | ||
| if data_offset >= 20 && tcp.len() >= data_offset { | ||
| let mut pos = 20; | ||
| while pos < data_offset { | ||
| let kind = tcp[pos]; | ||
| if kind == 0 { | ||
| break; | ||
| } | ||
| if kind == 1 { | ||
| pos += 1; | ||
| continue; | ||
| } | ||
| if pos + 2 > data_offset { | ||
| break; | ||
| } | ||
| let opt_len = tcp[pos + 1] as usize; | ||
| if opt_len < 2 || pos + opt_len > data_offset { | ||
| break; | ||
| } | ||
| if kind == 2 && opt_len == 4 { | ||
| info.tcp_mss = u16::from_be_bytes([tcp[pos + 2], tcp[pos + 3]]); |
| ) -> StageOutcome<()> { | ||
| if !screen.has_profiles() { | ||
| return StageOutcome::Continue(()); | ||
| } |
Agent-Logs-Url: https://github.com/psaab/xpf/sessions/07153275-84bc-4d85-9e90-bbb0cf25381c Co-authored-by: psaab <196946+psaab@users.noreply.github.com>
Adversarial review completed and I addressed the hot-path finding in |
Round-1 dual-review consolidated synthesis on
|
| Reviewer | Verdict |
|---|---|
| Codex | MERGE-NEEDS-MAJOR (3 MAJOR + 1 MINOR) |
| Gemini Pro 3 | MERGE-NEEDS-MAJOR (3 MAJOR) |
Both converge MERGE-NEEDS-MAJOR. Both independently caught the unbounded queue bug.
MAJOR 1 — Validated-client cache is NOT actually bounded (both reviewers)
userspace-dp/src/screen.rs:607-611 refreshes an existing key by updating entries and blindly pushing another SynCookieValidatedEntry into queue. entries.len() stays capped, but queue grows without bound under repeated valid ACKs for the same tuple.
Worked failure (Gemini, traced):
- T=0: insert(A) → queue=[(A,60)], entries={A:60}
- T=1: insert(B) → queue=[(A,60),(B,61)], entries={A:60, B:61}
- T=2..59: attacker replays/refreshes B at 1M PPS
- Each insert(B):
cleanup_expiredchecks front (A,60), not expired → break immediately entries.len() == 2 < capacity→evict_oldestnever calledqueue.push_backruns every iteration
- Each insert(B):
- T=59: queue has ~59M stale B entries → ~1.8 GB heap
Plus FxHashMap::default() at screen.rs:593 is not preallocated; first inserts allocate too.
MAJOR 2 — SynCookieChallenge is scaffolding, no end-to-end runtime (Codex)
screen.rs:888-891 returns the challenge, but userspace-dp/src/afxdp/poll_stages.rs:279-285 treats ScreenVerdict::SynCookieChallenge(_) exactly like a drop. No SYN-ACK TX path, no budget handling, no RST on validated ACK.
Plan admits this is deferred, but the code/tests create a challenge path that cannot work end-to-end if a master key is ever wired.
MAJOR 3 — Hot-path string allocation (Codex)
screen.rs:870, :873-875 allocates zone.to_string() on every over-threshold SYN-cookie SYN. Directly conflicts with plan invariant at plan-1374-syn-cookies.md:78 ("no heap allocation").
MAJOR 4 — HashDoS via FxHashMap (Gemini)
SynCookieValidatedCache::entries: FxHashMap<...> uses rustc-hash. FxHash is NOT DoS-resistant. This cache sits on the network edge mitigating SYN floods. An attacker can trivially craft tuples to collide into the same bucket, degrading O(1) → O(N) on the exact attack surface this gate is meant to protect against.
Fix: use std::collections::HashMap (SipHash, DoS-resistant) for any network-cache structure exposed to attacker-controlled input.
Production-still-rejected
Both reviewers note: capability gate at pkg/dataplane/userspace/manager.go:1112-1114 still rejects syn-cookie configs. No production caller wires update_syn_cookie_master_key. Current production is fail-closed; this PR is foundation only.
Plan-doc accuracy gaps
- Plan lists
screen::syn_cookie_budget_drop_does_not_starve_tx— doesn't exist - New tests exercise codec/cache helpers and direct
ScreenStatecalls, not full Go JSON snapshot → Rust deserialize → AF_XDP stage behavior
Recommendation
Block on:
- Fix unbounded queue: either bound
queuecapacity AND drop refresh-duplicates, OR cap total queue length and evict oldest on insert regardless of entries-cap - Switch
FxHashMaptoHashMap(SipHash) for the validated cache - Remove
zone.to_string()from the SYN cookie hot path (use&strreferences, interned IDs, or a fixed-size array) - Either implement end-to-end SYN-ACK TX or document the
SynCookieChallengeverdict as drop-equivalent until Feature gap: SYN cookie flood protection implemented in eBPF, missing from userspace-dp #1374's SYN-ACK TX slice lands
Codex task-mpa6l2zc-dau5vv; Gemini task-mpa6luek-2vqsmw. Not merging — author's decision.
| if protocol == PROTO_TCP | ||
| && (!info.is_fragment || info.is_first_fragment) | ||
| && let Some(tcp_start) = tcp_offset | ||
| && tcp_start + 20 <= frame.len() | ||
| { | ||
| let tcp = &frame[tcp_start..]; | ||
| info.tcp_seq = u32::from_be_bytes([tcp[4], tcp[5], tcp[6], tcp[7]]); | ||
| info.tcp_ack = u32::from_be_bytes([tcp[8], tcp[9], tcp[10], tcp[11]]); | ||
| let data_offset = ((tcp[12] >> 4) as usize) * 4; | ||
| if data_offset >= 20 && tcp.len() >= data_offset { | ||
| let mut pos = 20; | ||
| while pos < data_offset { | ||
| let kind = tcp[pos]; | ||
| if kind == 0 { | ||
| break; | ||
| } | ||
| if kind == 1 { | ||
| pos += 1; | ||
| continue; | ||
| } | ||
| if pos + 2 > data_offset { | ||
| break; | ||
| } | ||
| let opt_len = tcp[pos + 1] as usize; | ||
| if opt_len < 2 || pos + opt_len > data_offset { | ||
| break; | ||
| } | ||
| if kind == 2 && opt_len == 4 { | ||
| info.tcp_mss = u16::from_be_bytes([tcp[pos + 2], tcp[pos + 3]]); | ||
| break; | ||
| } | ||
| pos += opt_len; | ||
| } | ||
| } |
| - **Action**: #1373 Phase 1 documentation migration: mark Rust AF_XDP userspace as the primary/default dataplane development and validation target, demote eBPF wording to legacy compatibility/regression context, and preserve explicit retirement blockers for #1374-#1381 without claiming unresolved gaps closed. | ||
| - **File(s)**: `README.md`, `CLAUDE.md`, `docs/testing.md`, `docs/development-workflow.md`, `docs/test_env.md`, `docs/userspace-dataplane-gaps.md`, `docs/feature-gaps.md`, `docs/userspace-dataplane-architecture.md`, `docs/afxdp-packet-processing.md`, `docs/ha-cluster-test-plan.md`, `testing-docs/README.md`, `bpf/README.md`, `userspace-dp/README.md`, `pkg/dataplane/README.md`, `userspace-dp/src/afxdp/README.md`, `_Log.md` | ||
| - **Timestamp**: 2026-05-17T18:57:46Z | ||
| - **Action**: Adversarial PR #1374 follow-up: optimize SYN-cookie validated-client cache with map+queue state so insert/take are O(1), while expiry/eviction drain stale queue tokens from the head without whole-cache scans. |
Round-2 Gemini r2 review on
|
Round-2 dual-review consolidated synthesis on
|
| Reviewer | Verdict |
|---|---|
| Codex | MERGE-NEEDS-MINOR (missing production-hook test) |
| Gemini Pro 3 | MERGE-NEEDS-MINOR (missing plan-claimed test) |
Both converge MERGE-NEEDS-MINOR. All r1 MAJORs fully addressed.
r1 MAJOR resolution (both reviewers confirm)
- ✓ Unbounded validated-client state: replaced
VecDequescan path with fixed-size 4-way set table + keyed SipHash (screen.rs:620-753) - ✓ FxHashMap HashDoS surface: removed (set table uses SipHash)
- ✓ Hot-path zone allocation:
update_profilesprepopulates per-zone state (:790-804); packet path usesget_mut(zone)instead ofentry(zone.to_string())(:950-984) - ✓ Stale entries on key change:
update_syn_cookie_master_keyrekeys + clears admission table on key changes/unset (:813-822); regression test atscreen_tests.rs:1426-1459
Plan-doc + scope
Plan explicitly says this is validation/admission only, NOT full SYN-ACK/RST TX. Capability gate stays closed until TX + HA secrets + counters + integration validation land. FEATURES.md:23 matches the guarded state.
MINOR — Missing production-hook pin
Codex: nothing exercises stage_screen_syn_cookie_ack_on_session_miss at userspace-dp/src/afxdp/poll_stages.rs:297-354 or its session-miss call site at poll_descriptor.rs:462-475. A future refactor could orphan the production hook while all new screen unit tests still pass.
Recommended pins:
- Valid ACK recycle
- Invalid ACK screen-drop accounting
- Next-SYN admission
Gemini: plan-claimed tests don't all exist.
Recommendation
Block on: add stage-level production-hook test (Codex) + reconcile plan-claimed test list (Gemini).
Codex task-mpa7x7ku-z6bcmr. Gemini task-mpa7xoe4-of36bp. Not merging — author's decision.
|
@copilot review |
Addressed on subsequent commits:
This is now reconciled in
Added the production-hook pin in
Adversarial review complete on latest head ( |
Claude r3 review on
|
Round-3 triple-review synthesis on
|
| Reviewer | Verdict |
|---|---|
| Claude | MERGE-READY |
| Codex | MERGE-NEEDS-MINOR |
| Gemini Pro 3 | MERGE-NEEDS-MAJOR |
Real coverage gap on a security guard: r3 only tests the Validated happy path.
What's covered (r3 production-hook test)
- Calls real production stage
stage_screen_syn_cookie_ack_on_session_missatpoll_stages.rs:574-585(not a helper). - Asserts
StageOutcome::RecycleAndContinue, no false-positive drop counters, post-ACK SYN bypass + single-use consumption. - Cookie ISN dynamically extracted from production
SynCookieChallengeverdict — no magic-number assertion that could silently re-pass on MAC change.
What's NOT covered (Codex+Gemini agree)
The production stage maps three verdicts:
SynCookieAckVerdict::NotApplicable => StageOutcome::Continue(()),
SynCookieAckVerdict::Validated => StageOutcome::RecycleAndContinue,
SynCookieAckVerdict::Invalid => {
counters.touched = true;
counters.screen_drops += 1;
StageOutcome::RecycleAndContinue
}r3 covers only Validated. Missing at the poll-stage level:
- Invalid cookie path — the actual drop counter increment is unverified
- Cookie expiration —
TEST_NOW_SECSis static, never advances pastsyn_cookie_active_until_secs - Epoch rotation —
rg_epochsinitialized to 0, never rotated - 4-way associative MAC cache replacement — existing helper-level coverage at
screen_tests.rs:1344-1366, no production-stage coverage
My read
Codex MINOR vs Gemini MAJOR is a judgment call. This is a SYN flood security guard — verifying the drop-on-invalid path matters. Gemini's escalation reflects "core security guard with negative path unverified". Codex's MINOR reflects "the principal regression risk (validated path mis-handles drop counter) is covered".
I lean MINOR but the gap is real. A 30-LOC addition would close it.
Recommendation
Strongly consider in this PR: add poll-stage test for SynCookieAckVerdict::Invalid asserting counters.screen_drops += 1 and counters.touched = true. The fixture already builds challenges + ACKs; constructing an ACK with wrong cookie is a small delta.
Defer: expiration + epoch rotation + 4-way cache replacement poll-stage coverage as follow-up (helper-level coverage exists).
Codex task: task-mpaahys2-rtqa1l. Gemini task: task-mpaaiaod-dith2m. Not merging — author's decision.
|
@copilot review |
Added in
Adversarial re-review complete on current head |
| - **Action**: #1373 Phase 1 documentation migration: mark Rust AF_XDP userspace as the primary/default dataplane development and validation target, demote eBPF wording to legacy compatibility/regression context, and preserve explicit retirement blockers for #1374-#1381 without claiming unresolved gaps closed. | ||
| - **File(s)**: `README.md`, `CLAUDE.md`, `docs/testing.md`, `docs/development-workflow.md`, `docs/test_env.md`, `docs/userspace-dataplane-gaps.md`, `docs/feature-gaps.md`, `docs/userspace-dataplane-architecture.md`, `docs/afxdp-packet-processing.md`, `docs/ha-cluster-test-plan.md`, `testing-docs/README.md`, `bpf/README.md`, `userspace-dp/README.md`, `pkg/dataplane/README.md`, `userspace-dp/src/afxdp/README.md`, `_Log.md` | ||
| - **Timestamp**: 2026-05-17T18:57:46Z | ||
| - **Action**: Adversarial PR #1374 follow-up: optimize SYN-cookie validated-client cache with map+queue state so insert/take are O(1), while expiry/eviction drain stale queue tokens from the head without whole-cache scans. |
Claude r4 review on
|
Round-4 triple-review synthesis on
|
| Reviewer | Verdict |
|---|---|
| Claude | MERGE-READY |
| Codex | MERGE-NEEDS-MINOR |
| Gemini Pro 3 | MERGE-NEEDS-MAJOR |
Invalid-ACK path — CLOSED (all three agree)
The new test at poll_stages.rs +45 exercises the production stage with a 0xdead_beef ACK that triggers SynCookieAckVerdict::Invalid:
- Asserts
invalid_counters.touched— state was updated ✓ - Asserts
invalid_counters.screen_drops == 1— drop counter incremented ✓ - Asserts post-invalid SYN gets a fresh
SynCookieChallenge(NOT bypass) — security guard ✓
Codex: "the only production branch that increments screen_drops is SynCookieAckVerdict::Invalid at poll_stages.rs:346." Verified the test exercises that exact branch.
Remaining deferral — both reviewers want acknowledgment
The other three sub-gaps from r3 (expiration, epoch rotation, 4-way cache replacement) remain uncovered at the poll-stage. Helper-layer coverage exists at screen_tests.rs:1344-1366.
Codex MINOR: "Either add a short rationale for why those mechanics stay covered below the poll stage, or add the missing poll-stage cases."
Gemini MAJOR: "you cannot silently drop 75% of a documented MAJOR blocker without explanation." Demands either coverage or explicit deferral acknowledgment in commit/PR doc.
My read
Codex MINOR is the right framing. The most security-critical path (invalid ACK doesn't bypass) is now covered. The remaining three sub-gaps are:
- Expiration: time-based; tests would need clock advance
- Epoch rotation: cross-RG-state; needs HA fixture
- 4-way cache replacement: capacity-based; needs N-of-4 fixture
Each is a legitimate test but they're not silent-regression-prone the way the invalid-ACK happy-path-only test was (that one had the security-critical "don't install bypass" assertion missing).
Recommendation
Block on: add explicit comment near the new test (or in the commit message body / PR description) acknowledging the three deferred sub-gaps with a rationale (e.g., "helper-layer coverage at screen_tests.rs:1344-1366 pins MAC cache replacement; expiration + epoch rotation deferred to follow-up issue per #XXX"). Either reviewer's bar is then satisfied. Don't add the missing tests in this PR.
Codex task: task-mpabsekf-dwxwwi. Gemini task: task-mpabsnv2-2tvdyr. Not merging — author's decision.
|
@copilot review\n\nPushed |
|
Fix pushed for the r4 MINOR deferral-comment gap.\n\nCommit: 02fdb0f\n\nWhat changed:\n- Added explicit #1374 deferral notes for validated-client cache expiration, secret-epoch rotation, and HA/cache survivability in docs and the runtime hook comment.\n\nValidation:\n- cargo test --manifest-path userspace-dp/Cargo.toml session_miss_ack_stage_invokes_syn_cookie_runtime_validation\n- git diff --check\n\n@copilot review |
Addressed in
Adversarial re-review complete on |
Claude r5 review on
|
Round-5 triple-review synthesis on
|
| Reviewer | Verdict |
|---|---|
| Claude | MERGE-READY |
| Codex | MERGE-NEEDS-MAJOR |
| Gemini Pro 3 | MERGE-NEEDS-MINOR |
What r5 added
5 LOC across docs/userspace-dataplane-gaps.md, userspace-dp/src/FEATURES.md, userspace-dp/src/afxdp/poll_stages.rs. Both reviewers confirm operator-visible deferral acknowledgment in PR-discoverable docs.
Codex MAJOR — enumeration incomplete + no rationale + no test pointer
Codex:
"r5 names
validated-client cache expiration semantics/Cache expiryandsecret-epoch rotation, but it does not explicitly enumerate 4-way cache replacement as a deferred sub-gap. It only saysbounded validated-client cacheorHA/cache survivability, which is not the same thing."
"The new text does not point readers to the lower-layer coverage at
userspace-dp/src/screen_tests.rs:1344-1366."
"The poll-stage comment gives a scope statement... It does not give the missing technical rationale for skipping poll-stage coverage."
Gemini MINOR — enumeration sufficient, rationale missing
Gemini:
"The three missing sub-gaps are now explicitly listed. ... The missing rationale is a process gap but does not obscure the state of the codebase."
Disagreement on framing
The r4 sub-gaps were: expiration, epoch rotation, 4-way cache replacement.
r5 says: "validated-client cache expiration semantics, secret-epoch rotation, HA-safe secret publication/cache survivability".
- "Cache expiration" → expiration ✓
- "Secret-epoch rotation" → epoch rotation ✓
- "HA-safe cache survivability" → ??? Codex argues this is NOT the same as 4-way associative cache replacement; Gemini accepts it as equivalent.
Reading both: 4-way associative cache replacement is a capacity/eviction concern; HA cache survivability is a cross-process state replication concern. They are different invariants. Codex is right that 4-way replacement isn't enumerated.
Recommendation
Strongly consider in this PR:
- Add 4-way cache replacement as a distinct deferred sub-gap (currently subsumed under "cache survivability" which is a different invariant).
- Add pointer to
screen_tests.rs:1344-1366for lower-layer coverage. - Add one-sentence rationale for the deferral pattern (e.g. "Poll-stage tests cover the operational drop/bypass semantics; cache replacement and HA survivability are tested at the lower screen layer per X").
These are all comment/doc adjustments — minutes-level edits. Then both reviewers converge.
Codex task: task-mpad7300-35p3ha. Gemini task: task-mpad7byk-x33wd5. Not merging — author's decision.
|
Pushed Fix:
Validation:
|
| | File | Stage | What it does | | ||
| |------|-------|--------------| | ||
| | `screen.rs` | screen | Pre-session attack-protection checks (land, TCP SYN+FIN, no-flag, FIN-without-ACK, ICMP frag, plus rate-limits). Mirrors `bpf/xdp/xdp_screen.c`. Also contains the #1374 userspace SYN-cookie mint/validate core; the Go `syn-cookie` capability gate must stay closed until SYN-ACK TX replies, returning-ACK handling, HA-secret publication, bounded validated-client state, and status counters are wired into the worker path. | | ||
| | `screen.rs` | screen | Pre-session attack-protection checks (land, TCP SYN+FIN, no-flag, FIN-without-ACK, ICMP frag, plus rate-limits). Mirrors `bpf/xdp/xdp_screen.c`. Also contains the #1374 userspace SYN-cookie mint/validate core, fixed-size validated-client admission table, and session-miss ACK validation hook. The lower screen tests cover bounded 4-way validated-client cache replacement; validated-client cache expiration, secret-epoch rotation, and HA-safe secret/cache survivability are still deferred #1374 work. The Go `syn-cookie` capability gate must stay closed until SYN-ACK TX replies, ACK RST emission, HA-secret publication, status counters, and integration validation are wired into the worker path. | |
| - **Action**: #1373 Phase 1 documentation migration: mark Rust AF_XDP userspace as the primary/default dataplane development and validation target, demote eBPF wording to legacy compatibility/regression context, and preserve explicit retirement blockers for #1374-#1381 without claiming unresolved gaps closed. | ||
| - **File(s)**: `README.md`, `CLAUDE.md`, `docs/testing.md`, `docs/development-workflow.md`, `docs/test_env.md`, `docs/userspace-dataplane-gaps.md`, `docs/feature-gaps.md`, `docs/userspace-dataplane-architecture.md`, `docs/afxdp-packet-processing.md`, `docs/ha-cluster-test-plan.md`, `testing-docs/README.md`, `bpf/README.md`, `userspace-dp/README.md`, `pkg/dataplane/README.md`, `userspace-dp/src/afxdp/README.md`, `_Log.md` | ||
| - **Timestamp**: 2026-05-17T18:57:46Z | ||
| - **Action**: Adversarial PR #1374 follow-up: optimize SYN-cookie validated-client cache with map+queue state so insert/take are O(1), while expiry/eviction drain stale queue tokens from the head without whole-cache scans. |
Claude r6 review on
|
Round-6 triple-review synthesis on
|
| Reviewer | Verdict |
|---|---|
| Claude | MERGE-READY |
| Codex | MERGE-READY |
| Gemini Pro 3 | MAJOR — HALLUCINATION; verified false |
r5 MAJOR closure verified
Codex:
"A. Yes, substantively.
gaps.md:82andFEATURES.md:23explicitly name bounded/4-way validated-client cache replacement as its own covered cache concern, separate from the remaining deferred #1374 work.""B. Yes. The docs say poll-stage tests only pin operational invalid-ACK drop/bypass behavior, and the code comment in
poll_stages.rs:295-299explains that lower screen runtime owns cache mechanics.""C. Yes, with a small caveat. The docs point to
userspace-dp/src/screen_tests.rs; the actual lower-layer coverage issyn_cookie_validated_cache_is_boundedatscreen_tests.rs:1344-1366. They do not include literal line numbers, but the file-plus-behavior pointer is clear enough."
Codex independently confirmed the named test exists at the claimed location.
Gemini MAJOR — VERIFIED FALSE
Gemini claimed:
"FAILED / HALLUCINATED. The file userspace-dp/src/screen_tests.rs only has 1213 lines total in the repository. Lines 1344-1366 do not exist. A grep for
cacheandvalidateinscreen_tests.rsyields 0 results. The author claimed coverage that was either completely hallucinated or accidentally omitted from the PR branch."
Direct verification at 16269c4f:
git show 16269c4f:userspace-dp/src/screen_tests.rs | wc -l→ 1798 lines (NOT 1213 as Gemini claimed)- Lines 1344-1366 contain the function
syn_cookie_validated_cache_is_bounded(verified bygit show ... | sed -n '1344,1366p') - The function constructs
SynCookieValidatedCache::new(4, 64), inserts 32 entries, assertscache.len() == 4(bounded), then asserts the FIFO replacement order is correct
So Gemini's KILL/MAJOR verdict is the third hallucination on this codebase: claimed file was 1213 lines (it's 1798), claimed the test doesn't exist (it does at the exact location), claimed grep yielded 0 results (the test name contains "validate" and "cache" both).
Recommendation
Merge-ready. Codex MERGE-READY verdict stands. Gemini hallucination dismissed.
Pattern continues per feedback_gemini_lazy_reviews.md: Gemini Pro 3 on this codebase issues MAJOR/KILL with manufactured rationale when the actual code is fine. Dropping the verdict.
Codex task: task-mpaljhsj-gq1an3. Gemini task: task-mpaljpd3-wd7xuw (hallucinated MAJOR — dismissed). Not merging — author's decision.
Adds guarded SYN cookie runtime path (#1409) in screen. - screen: avoid full scan in syn-cookie validated cache - screen: make syn-cookie validated cache O(1) lookup - screen: bound SYN cookie runtime state - test: cover syn-cookie ack poll stage - test: cover invalid SYN cookie poll-stage ACKs - docs: note SYN cookie runtime deferrals Touches userspace-dp/src/screen.rs, userspace-dp/src/screen_tests.rs, userspace-dp/src/afxdp, and docs/pr/1373-retire-ebpf-dataplane across documentation, tests, userspace dataplane, and dataplane integration. The largest file deltas are userspace-dp/src/screen.rs, userspace-dp/src/screen_tests.rs, and userspace-dp/src/afxdp/poll_stages.rs. The diff is 1242 additions and 42 deletions across 13 files.
Summary
Refs #1374.
Adds guarded userspace SYN-cookie runtime scaffolding while keeping the capability gate closed:
syn_cookiescreen profile state from Go snapshots into RustScreenStateThis is not full #1374 closure. SYN-ACK TX allocation/emission, RST emission for valid ACKs before session materialization, HA-safe secret publication/rotation, counters/status, and capability-gate removal remain open.
Validation
cargo test screen::go test ./pkg/dataplane/userspace -run 'TestUserspaceSupportsScreenProfiles|TestDeriveUserspaceCapabilities.*SynCookie|TestBuildScreenSnapshots'git diff --check