You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Every #[cfg(test)] mod tests under crates/frankenrust-core/src/
compiles into one lib test binary, and they all share the
process-global CONTEXT_SLOTS table (context.rs's pub static CONTEXT_SLOTS). Two tests that pick the same thread_index therefore
race over one slot.
Nothing enforces that they don't. The only defence is a hand-maintained
prose comment above each file's index block, saying which ranges the other
files use. That is five comments that must be kept in sync by hand, and at
least one of them is already wrong.
The concrete wrongness today
callbacks/input.rs:274-277 says its range is 120-127, and its named IDX_* block (input.rs:278-285) does stop at 127. But four more indices
are declared as function-localconst IDX inside test bodies:
So input.rs's real range is 120-131, and neither its own comment nor output.rs:410-413's cross-reference says so.
What it already cost
#169 added tests that claimed 130 and 131 on the strength of that comment.
The result was an intermittent red gate: input.rs's set(130, ..)
replaces the worker test's context and drops its counting CompletionSignal unfired, so finish_php_request_closes_the_context_fires_the_signal_once_and_leaves_the_slot_installed
fails its fire_count == 1 assertion. Measured at 1 failure in 300 runs
of the filtered set at --test-threads=8 (two reviewers independently
measured 10/400 and 2/60). The full suite passed 300/300, which is why the
gate went green -- the margin is an accident of the current test count.
That collision is fixed in #169's own file (moved to 132-134). The mechanism is not fixed, and the next agent to add a slot-using test
reads the same wrong comment.
Note the quieter failure direction too: when the replacement context is
the one installed, input.rs's two tests still assert is_null() and pass vacuously. A collision can hide a real regression as easily as it
can invent a fake one.
What to build
Replace the convention with a single enforced allocation point.
Add one #[cfg(test)]-only module -- suggest pub(crate) mod test_slots in crates/frankenrust-core/src/context.rs, next to CONTEXT_SLOTS itself, or a new src/test_slots.rs gated on #[cfg(test)] -- holding one named pub(crate) const per test that
needs a slot, for all of misc.rs (1-4), servervars.rs (60-76), output.rs (100-116), input.rs (120-131) and callbacks/worker.rs
(132-134).
In that module, list every constant once more in a const ALL: &[(&str, usize)] and add a #[test] that asserts the
values are pairwise distinct, naming both colliding constants in the
failure message. This is the part that makes it permanent: a future
collision becomes a deterministic red test with a clear message instead
of a sub-1% flake in an unrelated file.
Repoint all five test modules at test_slots::* and delete the
per-file range comments, including the four function-local const IDX
declarations in input.rs, which are the ones the comments missed.
No usize slot-index literal remains inside any crates/frankenrust-core/src/callbacks/*.rs test module; they all name
a test_slots constant.
The uniqueness test exists and fails (with both names in the message)
when two constants are given the same value. Verify by temporarily
duplicating a value; do not commit that.
rg -n 'usize = [0-9]+' crates/frankenrust-core/src shows slot indices
only in the new module.
Gate: default
Agent: codex
Every
#[cfg(test)] mod testsundercrates/frankenrust-core/src/compiles into one lib test binary, and they all share the
process-global
CONTEXT_SLOTStable (context.rs'spub static CONTEXT_SLOTS). Two tests that pick the samethread_indexthereforerace over one slot.
Nothing enforces that they don't. The only defence is a hand-maintained
prose comment above each file's index block, saying which ranges the other
files use. That is five comments that must be kept in sync by hand, and at
least one of them is already wrong.
The concrete wrongness today
callbacks/input.rs:274-277says its range is120-127, and its namedIDX_*block (input.rs:278-285) does stop at 127. But four more indicesare declared as function-local
const IDXinside test bodies:input.rs:408-> 128 (go_read_cookies_percent_decodes_...)input.rs:422-> 129input.rs:434-> 130 (go_read_cookies_with_no_cookie_header_returns_null)input.rs:446-> 131 (go_read_cookies_with_a_single_empty_cookie_value_returns_null)So input.rs's real range is 120-131, and neither its own comment nor
output.rs:410-413's cross-reference says so.What it already cost
#169 added tests that claimed 130 and 131 on the strength of that comment.
The result was an intermittent red gate:
input.rs'sset(130, ..)replaces the worker test's context and drops its counting
CompletionSignalunfired, sofinish_php_request_closes_the_context_fires_the_signal_once_and_leaves_the_slot_installedfails its
fire_count == 1assertion. Measured at 1 failure in 300 runsof the filtered set at
--test-threads=8(two reviewers independentlymeasured 10/400 and 2/60). The full suite passed 300/300, which is why the
gate went green -- the margin is an accident of the current test count.
That collision is fixed in #169's own file (moved to 132-134). The
mechanism is not fixed, and the next agent to add a slot-using test
reads the same wrong comment.
Note the quieter failure direction too: when the replacement context is
the one installed,
input.rs's two tests still assertis_null()andpass vacuously. A collision can hide a real regression as easily as it
can invent a fake one.
What to build
Replace the convention with a single enforced allocation point.
#[cfg(test)]-only module -- suggestpub(crate) mod test_slotsincrates/frankenrust-core/src/context.rs, next toCONTEXT_SLOTSitself, or a newsrc/test_slots.rsgated on#[cfg(test)]-- holding one namedpub(crate) constper test thatneeds a slot, for all of
misc.rs(1-4),servervars.rs(60-76),output.rs(100-116),input.rs(120-131) andcallbacks/worker.rs(132-134).
const ALL: &[(&str, usize)]and add a#[test]that asserts thevalues are pairwise distinct, naming both colliding constants in the
failure message. This is the part that makes it permanent: a future
collision becomes a deterministic red test with a clear message instead
of a sub-1% flake in an unrelated file.
test_slots::*and delete theper-file range comments, including the four function-local
const IDXdeclarations in
input.rs, which are the ones the comments missed.ContextSlots::slotgrows thetable one entry per index up to the one asked for and never reclaims
(see core: ContextSlots::slot grows the slot table densely and unboundedly, and a wrong thread index costs 355 MB with a green gate #112, and the measurement in
misc.rs's tests module comment:~800k costs 362 MB of peak RSS). Reuse the existing 1-134 numbering
rather than renumbering into tidy per-file blocks with gaps.
Acceptance
usizeslot-index literal remains inside anycrates/frankenrust-core/src/callbacks/*.rstest module; they all namea
test_slotsconstant.when two constants are given the same value. Verify by temporarily
duplicating a value; do not commit that.
rg -n 'usize = [0-9]+' crates/frankenrust-core/srcshows slot indicesonly in the new module.
bash scripts/gate.sh defaultpasses.Out of scope
ContextSlots::slotsparse or bounded). Thisissue only keeps the test indices small; it does not change
ContextSlots.context.rs's own tests, which build a localContextSlots::new()andnever touch the global table.