Commit e9e5d15
fix(PocketIC): don't set a hard TTL by default (#11167)
## What
`PocketIc::new` / `PocketIcBuilder::build` start the PocketIC server
with
`hard_ttl: Some(600s)`. This PR sets `hard_ttl: None` instead, restoring
the
server's own default of no hard TTL.
## Why
The hard TTL is an **absolute deadline measured from the server's
launch**. Unlike
the soft TTL it is never extended by activity (`started` is stamped once
and never
reset) and it does not consult `pending_requests`, so it terminates
servers that are
actively serving requests — via `std::process::exit(124)`, which
bypasses the
graceful `terminate(...)` path and resets every open connection.
Crucially, **the 600s is a budget for an entire test binary, not for one
test**: with
`reuse: true` the port file is keyed on the test driver's PID
(`packages/pocket-ic/src/lib.rs:2360`), so all tests in a binary share
one server.
Any suite whose *total* runtime crosses 10 minutes has its server shot
out from under
it, and every in-flight call panics at the non-retrying
`.expect("HTTP failure")` (`nonblocking.rs:1737`):
```
HTTP failure: reqwest::Error { kind: Request,
url: ".../instances/9/update/await_ingress_message",
source: ... Os { code: 104, kind: ConnectionReset, message: "Connection reset by peer" } }
```
### Evidence
This is currently the top source of flakiness in
`//rs/nervous_system/integration_tests:sns_lifecycle` — 14 flaky runs in
one week.
Each flaky run has a failed attempt and a passing retry, which gives a
clean paired
comparison:
| attempt | `finished in …` |
| --- | --- |
| **FAILED** (14/14) | 602.0, 602.3, 602.5, 602.6, 602.8, 603.3, 603.4,
603.8, 605.1, 606.2, 608.4, 611.4, 613.0 s |
| **PASSED** (14/14) | 245.3, 246.5, 248.9, 248.9, 251.0, 261.4, 262.3,
263.8, 264.7, 265.2, 265.9, 325.9, 394.3, 439.6 s |
Zero overlap; the boundary is exactly 600s. The suite passes if and only
if it fits
inside the hard TTL.
It presents misleadingly: 13 of 14 failures name a
`test_sns_lifecycle_swap_timeout_*`
test, but those three scenarios merely sort last alphabetically and so
are what is
still running when the deadline lands. The number of tests that fail
equals the number
in flight at t=600s — in one run on a slower machine an earlier batch
was caught and
all four of *its* tests failed simultaneously at the identical call
site, including
happy-path ones.
Note also that the Bazel target is `timeout = "long"` (900s) while the
hard TTL is
600s: the safety net fires *before* the timeout the test is budgeted
against.
## Why remove rather than raise
The hard TTL was introduced in #8997 to prevent orphaned server
processes. Removing
it rather than bumping it, because:
1. **It does not do what it is credited with.** Neither `terminate()`
(`rs/pocket_ic_server/src/main.rs:401`) nor `exit(124)` kills the
sandbox
launcher / canister-sandbox processes — there is no process-group
handling
anywhere in `rs/pocket_ic_server/src/`. Those children are orphaned and
keep the
inherited stdout/stderr write-ends open, which is the actual failure
mode on RBE
runners that wait for pipe EOF. Killing the server does not release
them; only a
process-group kill does (which is what #10751 adds).
2. **Orphaned servers are already bounded by the soft TTL** — 60s,
activity-based.
When a parent dies its server goes idle immediately, so the soft TTL is
both
sufficient and *tighter* than the hard TTL for that case.
3. **The zombie-accumulation motivation no longer applies.** It came
from persistent
self-hosted macOS runners; those have been replaced by ephemeral
namespace.so
runners, which are torn down after each job.
### What this gives up
The one case the hard TTL uniquely covered is a *hung* server, where
requests never
complete so `pending_requests` never returns to zero and the soft TTL
can never fire.
Such a server now runs until something else reaps it (runner teardown,
Bazel/job
timeout). That seems the right trade — it is rare, bounded by outer
timeouts, and
better fixed at the hang than by terminating healthy servers on a timer
— but it is a
deliberate choice rather than an oversight.
### Escape hatch
`StartServerParams::hard_ttl`, the `--hard-ttl` CLI flag and the
server-side
implementation are all unchanged. Callers who want a hard TTL can still
set one via
`start_server` and pass the URL to `PocketIcBuilder::with_server_url`.
## Relation to #10751
Complementary, no dependency either way. #10751 makes the spawning
process own the
server and process-group-kill it at exit, which is the proper
replacement for the
orphan safeguard and what makes "no hard TTL" belt-and-braces on unix.
It does not
overlap with this change: it touches neither `nonblocking.rs` nor
`rs/pocket_ic_server/src/main.rs`, and its `atexit` reaper runs only
once all tests in
a binary have finished — after the point where this bug bites. This PR
stands on the
soft TTL alone.
## Impact on external users
`pocket-ic` is published, so this is user-visible and is recorded under
`## Unreleased`
in `CHANGELOG.md`. It should be a fix for downstream users too: any test
suite over ten
minutes has been hitting this same cliff silently.
## Testing
- `cargo check --all-targets --all-features -p pocket-ic` — clean.
- `cargo fmt` + `./ci/scripts/rust-lint.sh` — clean (exit 0).
- 151 Bazel test targets transitively depend on `packages/pocket-ic`;
these have **not**
been run locally and are left to CI. Note that a green
`//rs/nervous_system/integration_tests:sns_lifecycle` does not by itself
demonstrate
the fix, since that test passes today whenever it finishes under 600s —
the
meaningful check is that the failure mode is no longer reachable at all.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>1 parent 51ddfcf commit e9e5d15
2 files changed
Lines changed: 7 additions & 4 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
10 | 10 | | |
11 | 11 | | |
12 | 12 | | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
13 | 19 | | |
14 | 20 | | |
15 | 21 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
49 | 49 | | |
50 | 50 | | |
51 | 51 | | |
52 | | - | |
53 | | - | |
54 | | - | |
55 | 52 | | |
56 | 53 | | |
57 | 54 | | |
| |||
156 | 153 | | |
157 | 154 | | |
158 | 155 | | |
159 | | - | |
| 156 | + | |
160 | 157 | | |
161 | 158 | | |
162 | 159 | | |
| |||
0 commit comments