Registering with a new callback_url previously left the old Stalwart
subscription live alongside the new one, so Stalwart pushed the same mail
event to both origins and the user got duplicate notifications. The
per-URL mapping had no record of which subscription was current.
Add a push:registration:current pointer written alongside the per-URL
mapping. register_push now retires the previous subscription under the
existing registration lock before creating the new one: destroy first,
then delete the mapping, then mark the status disabled. A destroy failure
returns a retryable 503 (push_destroy_failed) rather than proceeding
silently - silently skipping is exactly the shape of the original bug.
The idempotency short-circuit still runs first, so re-registering the
same URL never touches the retire path. disable_push clears the pointer
only when the disabled URL is the current one.
Extracts destroy_push_registration, shared by disable_push and the new
retire path. Telegram needs no change: setWebhook replaces the previous
webhook atomically.
Adds 5 tests with a recording mock that captures the create/destroy
call order: idempotent no-recreate, retire-on-change, abort-without-
create when the old destroy fails, skip-destroy when the previous
mapping expired, and pointer untouched when disabling a non-current URL.
A remote TLS readback arrives after most of the 1s floor has been consumed,
so the +/-500 ms window could never be met (PTTL -2 / 269 / 118 ms across
three runs). PTTL -1 is what actually signals the regression this assertion
guards against -- EX silently dropped, key lives forever -- and a live value
can only be counting down, so bound it from above.
SET ... NX EX 1 itself is unchanged; the expired (-2) outcome is what proved
it took effect.
Bare TTL literals at call sites were invisible to the test suite:
MemoryState ignores the TTL argument entirely, so shrinking a 24h dedup
window to 60_480 compiled and stayed green. All 17 values now live in
pub(crate) mod ttl, the call sites reference them, and
ttl_contract_is_pinned asserts each value plus two ordering invariants
(heartbeat shorter than the lock it renews; consent tiers strictly
increasing).
The one value the earlier sweep could not reach is now reached: the EXPIRE
inside retry_or_dlq's Lua script is built with an interpolated
ttl::RETRY_COUNTER_SECONDS, so the script carries no second literal.
Gate: fmt/check/clippy clean; cargo test --locked 89 passed / 0 failed /
4 ignored (the ignored ones need REDIS_TEST_URL or JMAP_TEST_URL).
Not claimed: the two real-Redis PTTL assertions are wired but cannot run
today. Both Upstash tokens I have are rejected with -WRONGPASS, so
connect() fails before any PTTL is read.
The two REDIS_TEST_URL-gated tests could never have passed as written: a
unit-test binary never runs main(), so no rustls crypto provider was
installed and the TLS handshake to a rediss:// URL panicked before any
command was sent. Install the provider in a helper inside the test module
and reuse the same call main.rs already makes.
Also fix a key-construction typo in the retry/DLQ test: the retry counter
key embeds the message id, not the run id.
Verified against the live Upstash instance:
REDIS_TEST_URL=rediss://... cargo test --locked -- --ignored ttl
ttl_claim_dedup_sets_the_exactly_requested_expiry ... ok
ttl_consent_and_retry_landing_on_real_redis ... ok
Two latent bugs surfaced while preparing to run these against Upstash:
1. The retry key was mis-constructed. builds its counter
as , so the test had to read
. It read
, a key the call never writes, so the
assertion would have failed on the first real run (PTTL of a missing
key is -2, and the test's own guard rejects it).
Derive the key from the same stream name and message id the call
receives instead of re-spelling it by hand.
2. The consent probe used the process id as a Telegram chat_id. A
private Telegram chat_id is a plain positive integer, so a small pid
can legitimately be a real user. If the test is pointed at the live
database it would have overwritten that user's AI consent flag with a
1h expiry. Anchor at -4e18 + pid instead: real Telegram ids stay
under ~1e11 in magnitude, so no live flag can be shadowed.
All test keys remain prefixed and are deleted at the end of each test.
real_redis_ttl_tests in src/state.rs closes the "no test asserts any TTL
value" gap for the three atomic write points that actually reach Redis:
- set_nx_ex SET NX EX, with .max(1) floor (Redis rejects a zero EX)
- retry_or_dlq EXPIRE KEYS[1] 86400, embedded in the Lua script
- set_ai_consent SET EX
Each is opt-in via REDIS_TEST_URL (#[ignore], mirrors the JMAP gating in
src/domain/jmap/client.rs:522); missing the env var prints 'skipped' and
returns, so CI stays green without credentials and the URL is never logged.
retry_or_dlq uses max_attempts=99 so the script returns before XADD/XACK and
no stream or group needs to be created.
opengaps.md item 2 is now blocked only on a reachable Redis URL. Two
defects in that section were fixed while touching it: the consent table
rendered 7 * 86_400 as 604_800 and 365 * 86_400 as 31_536_000 (arithmetic
right, literal wrong) and listed 300s/7200s values that do not exist in
the code - the sixth real consent TTL is 0 (revoke). 16 line anchors had
also drifted onto comments and neighbours; all are re-pointed. The line-
range audit passes 138 refs but only checks that a line is non-blank, not
that it holds the claimed content, which is how these slipped through.
GATE-P0 baseline moves to 88 passed / 0 failed / 4 ignored.
The previous commit switched to ConnectionManager but took its config
defaults. Two of them are unusable.
Default number_of_retries is 6, which is not a bounded retry in redis
0.27.6. Measured with a throwaway binary against a refusing host
(127.0.0.1:1, ConnectionManager::new_with_config):
retries = 0 -> returns in ~0.5 ms with IoError
retries = 1 -> returns in ~1.5 s with IoError
retries = 6 -> never returns (still hung at 300 s)
So the crate default is an infinite loop, not "six tries". With it in
place a Redis host that is down at boot makes startup hang forever: no
error, no log, and a container that reports Up while serving nothing.
The code path this replaced (get_multiplexed_async_connection) returned
in about half a millisecond, so the default quietly turned a fail-fast
boot into a silent hang.
Default connection_timeout is None, which leaves each attempt at the OS
connect timeout: roughly two minutes for a host that drops packets
instead of refusing them.
Setting retries to 1 restores fail-fast boot -- an unreachable host now
exits in under two seconds with `State(Redis(Connection refused))` --
while still absorbing a single transient refusal. Anything more is
redundant here: the manager re-enters the connect path on every
subsequent command once the connection drops, so recovery never depended
on one operation retrying many times.
Measured end-to-end against a local redis container, killing the Redis
and restarting it while the app stayed up:
before (retries 6): 61.6 s to recover, and during the outage
/debug/redis hung past a 25 s curl timeout
after (retries 1): 3.0 s to recover, and /debug/redis answers
immediately with detail=io_error
Auto-recovery still works with zero restarts; this only makes it faster
and stops the health endpoint from wedging while Redis is down.
Both values are named constants with the measurements in their docs so
the numbers can be checked and the "why only one retry" question does
not have to be answered again.
Gates: fmt clean, clippy -D warnings clean, cargo test 87 passed,
2 ignored, unchanged from the previous baseline.
RedisState built one MultiplexedConnection at startup and kept it for the
life of the process; every call cloned the handle, which clones the handle
and not the socket. When the underlying TCP connection dropped -- an Upstash
idle timeout, a network blip, an idle-connection cutoff -- the handle was
permanently dead. The process kept running, every Redis read failed, and
only a restart recovered it. That is what had to be restarted.
The crate says so in its own docs: redis 0.27.6 lib.rs:71 recommends
ConnectionManager "for automatic reconnections" behind the
connection-manager feature, which we had never enabled.
RedisState now holds a ConnectionManager built from the Client. Both types
derive Clone and implement ConnectionLike, so the five query_async call
sites are untouched; the three helper signatures that took a cloned
connection by value or by mutable reference change type only.
MultiplexedConnection no longer appears anywhere in src/.
Start-up semantics are unchanged: ConnectionManager::new awaits
new_connection before returning, so an unreachable Redis still fails closed
at boot rather than succeeding and erroring on first use. Reconnect is gated
on is_io_error(), which is exactly kind() == IoError. A dropped socket is an
IoError, so it reconnects with exponential backoff and jitter (base 2,
100ms factor, 6 retries); an authentication failure is AuthenticationFailed
and is deliberately not retried, because that error does not fix itself.
No Drop or cancel is needed: the live connection sits behind
Arc<ArcSwap<SharedRedisFuture>>, the Client is retained for rebuilding, and
no detached control task is spawned, so nothing leaks when RedisState is
dropped and the existing #[derive(Clone)] still holds.
/debug/redis collapsed every failure into the single string
"redis_probe_failed", which is why the real error was invisible and the
outage took a session to trace. StateError::Redis carries the underlying
RedisError, so the class is readable here: authentication_failed,
io_error, busy_loading, client_error, response_error, plus
state_lock_poisoned and encryption_failed. ErrorKind is #[non_exhaustive],
so the match keeps a fallback arm for future variants.
No NotReachable variant exists in redis 0.27.6 -- a network outage
surfaces there as IoError or ClientError -- so the classifier does not
mention one.
Four new transitive dependencies come in with the feature: backon 1.6.0,
fastrand 2.4.1, futures 0.3.32, futures-executor 0.3.32.
Not covered: there is no test for the automatic reconnect path, which would
require tearing down a real TCP connection mid-flight. /debug/redis still
probes through is_enabled() rather than issuing a bare PING.
Gates: rust fmt clean, clippy --all-targets -D warnings clean,
cargo test 87 passed 2 ignored, unchanged from the previous baseline.
Every member's audit is now read and each real finding fixed, not just reported.
AGENTS.md §2.1
- The 500-line soft cap now has a machine-checkable exception: an over-limit
file must carry a one-line `SPLIT-EVAL:` marker stating the split decision.
- Six files were over the cap with no justification: notify.rs 2036, state.rs
1586, worker.rs 1045, config.js 1079, client.rs 601, config.rs 521. All now
carry the marker. client.rs was missed by my own earlier line count, which
did not descend into src/domain/jmap/.
- New validator scripts/docs_check/check_file_size.py; GATE-DOCS 4/4 -> 5/5.
It proves the marker exists, not that the written reason is sound.
- Re-based 144 line anchors across six docs for the +1 line each file gained;
every shifted anchor verified against the code, not just for non-blankness.
audit-arch.md
- design.md §6.1 module matrix omitted the `debug` module entirely; added.
- Seven numeric duplications in design.md now defer to reference.md §6.1 as
the single authority instead of restating the values.
- reference.md §5.1 was missing DEBUG_TOKEN; §5.3 listed six recognised names
and guessed `JMAP_*`. It now lists all 20 exactly as config.rs reads them.
- Corrected two wrong claims in the audit itself: debug.rs is compiled
unconditionally, not under #[cfg(debug_assertions)] (there are no cargo
features at all), and /debug/* exposes 7 routes, not the invented session
list / resend / rebuild surface.
audit-fileplanner.md
- reference.md cited a nonexistent `jmap_service.rs`; replaced with the real
adapter chain worker.rs -> domain/jmap.rs -> domain/jmap/client.rs.
- cloudflare-worker/README.md was outside every validator's scope; all four
doc validators now include it (208 section refs, 250 line refs).
- README.zh-CN.md stated only half of what README.md says about RUN_MODE; the
forward-placeholder sentence is now bilingual.
- charter.md §7 boundary table was missing rows for itself and for web/.
audit-rust.md / audit-frontend.md
- Error envelope corrected: `error` is a bare machine code, no `message`.
- Status table, AI-consent labels and ~15 stale or phantom facts fixed against
notify.rs; `TelegramClient::SendMessage` and `web_config` phantoms removed.
- GET|PUT /api/enabled forwarded by the worker so the SPA kill switch works.
- Ten UI strings that called the admin credential the Redis ACL password now
say CONFIG_ENCRYPTION_KEY.
- POST /healthz returns 405; dead HTTP_STATUS removed.
- audit_anchors.py now flags identifiers that do not exist in the file they
are anchored to.
charter.md
- `SAF-ENABLE-FLAG` registered (85 IDs); `SAF-ADMIN-SESSION` row corrected;
- `REQ-PUSH-TYPES` registered; the stale `admin_session_digest` removed.
Gates: GATE-DOCS PASS 5/5 (250 line-refs, 208 section refs, 0 errors);
GATE-P0 62 passed / 0 failed / 1 ignored, baseline unchanged; worker and SPA
36 tests, 0 failures.
Real Stalwart / Telegram / managed Redis verification stays blocked on missing
credentials and remains labelled as unverified.
- worker/state: paginate Email/changes by widening maxChanges on the same
sinceState (jmap-client 0.4.2 has no upToId) to avoid skipping a batch when
advancing per page; make dedup+enqueue atomic via claim_dedup_and_enqueue
(Lua SET NX EX then XADD); scale the XAUTOCLAIM idle threshold by batch size;
XACK payload-less malformed stream entries out of the PEL via ack_malformed;
fail closed on unknown streams (Err); tighten SET NX EX TTL to .max(1);
derive the CSPRNG fallback owner-token from time+PID+counter instead of a
constant; clear the push verification digest on push:disable
- worker: /help menu now lists only the consent triggers the parser accepts
- jmap/config: complete client and configuration validation
anonsaber