fix(redis): cascade-safety timeouts + per-command observability + cross-engine doc parity - #137
Merged
Merged
Conversation
…n resource
Adds five parameters to the redis_connection resource schema:
dial_timeout_ms default 2000 TCP dial
read_timeout_ms default 2000 per-command read (primary cascade knob)
write_timeout_ms default 2000 per-command write
pool_timeout_ms default 2000 wait for free pool conn
pool_size default 0 0 = client default (10*GOMAXPROCS)
These are wired through redisOptionsFromParams (split out so tests can
verify the mapping without miniredis) into go-redis's *redis.Options.
Background — 2026-06-22 tipsy-recsys incident:
A brief Redis hiccup (PING p99 ~970ms for ~5min) triggered a 30-minute
production outage. Pre-fix the resource constructed redis.NewClient with
only Addr/Password/DB, inheriting v9 defaults (read/write 3s, dial 5s,
PoolSize 10*GOMAXPROCS). With 2 vCPU containers PoolSize=20, which the
incident exhausted (Redis Pool Connections grafana panel went 5→20 the
moment PING latency rose). Each /execute request stuck in fetch_realshow
or filter_blocked_creators redis calls held its goroutine + RowFrame +
OperatorOutput in heap until the 10s HTTP timeout. Upstream retried,
the goroutine pile-up doubled, heap_inuse climbed RSS = Go sys = 3.87
GiB until OOM. Goroutine sched latency P99 hit 117ms (heap-pressured
runtime, not GC — GC frequency actually dropped because alloc rate
fell as goroutines parked on Redis I/O).
Why 2s defaults rather than 500ms:
The defaults must not regress healthy deployments. 2s is below v9's
3s/5s but well above any healthy steady-state (recsys's measured
PING p50 is sub-ms; even ZRANGEBYSCORE on 24h impression history is
single-digit ms). Operators that have measured their server's healthy
P99 should configure tighter values explicitly — recsys is moving to
500ms read/write under a separate change.
The redis operators (transform_redis_get/set, transform_redis_zrangebyscore
and recsys's own transform_query_blocked_creators) all default
fail_on_error=false, so a ctx.DeadlineExceeded from these timeouts
degrades the call into a cache-miss + warning rather than failing the
request. Cascade containment is at the resource boundary; downstream
business logic continues with cache-miss defaults.
Test:
- TestRedisOptionsFromParams_Defaults: all 4 timeouts land at 2s,
pool_size=0 preserves client default behaviour, metrics_name="".
- TestRedisOptionsFromParams_Explicit: every user-supplied value
flows through to *redis.Options verbatim (the regression test
that would have caught the original gap).
- TestRedisOptionsFromParams_RequireAddr: addr-required guard.
- Existing miniredis tests still green.
Refs: 2026-06-22 incident postmortem (recsys for_you).
…ion resource Mirrors the pine-go change: adds dial_timeout_ms / read_timeout_ms / write_timeout_ms / pool_timeout_ms / pool_size to the redis_connection resource schema, all with default 2000 ms (pool_size default 0 = pool default), and threads them through to JedisPool via DefaultJedisClientConfig + GenericObjectPoolConfig. Jedis-specific shape notes: - Jedis exposes connect timeout and a single socket timeout (no separate read/write knobs). write_timeout_ms is accepted in the schema for cross-engine parity but mapped to max(read_timeout_ms, write_timeout_ms) for the underlying socket so a strict write_timeout below the read deadline doesn't silently turn long-running reads (LRANGE / ZRANGEBYSCORE) into write failures. - pool_timeout_ms maps to GenericObjectPoolConfig.setMaxWait. pool_size maps to setMaxTotal, with setMaxIdle lifted to the same value when pool_size > 0 so ephemeral peaks aren't immediately dropped after use; pool_size=0 leaves the commons-pool2 default in place. - Pre-fix the resource constructed JedisPool with a hard-coded 2000ms connect timeout and no socket timeout, plus the default JedisPoolConfig (maxTotal=8). socket-timeout=∞ was the dominant cascade gap on Java just as v9 default 3s was on Go. Test: schemaExposesCascadeSafetyParams locks the cross-engine-shared defaults (2000 ms × 4, pool_size=0) on the registered schema, matching pine-go's TestRedisOptionsFromParams_Defaults. Refs: 2026-06-22 incident postmortem (recsys for_you).
…on resource
Mirrors the pine-go and pine-java changes: adds dial_timeout_ms /
read_timeout_ms / write_timeout_ms / pool_timeout_ms / pool_size to
the redis_connection resource schema. read/write timeouts use
SO_RCVTIMEO/SO_SNDTIMEO; dial_timeout uses a nonblocking-connect +
select() loop so it actually bounds the round trip rather than
relying on the kernel's TCP retransmit fallback.
C++-specific shape notes:
- Pre-fix the redis Client hard-coded a 2s socket timeout on both
read and write, and connect() was blocking with no application
timeout (subject to the kernel default ~75-130s). dial_timeout is
now applied via the standard nonblocking-connect dance: switch the
fd to O_NONBLOCK, fire connect, wait on writable with select bounded
by the deadline, check SO_ERROR, restore blocking.
- pool_timeout_ms is accepted on the schema for cross-engine parity
but is currently a no-op in C++. The pool's acquire never blocks
on a borrow — it constructs a fresh Client when the per-key idle
queue is empty — so there is no wait deadline to bound. Documented
on the schema; if a future refactor caps total concurrent
connections, this knob becomes the wait deadline.
- pool_size maps to the per-key idle-queue cap (was hard-coded
kMaxIdlePerKey=16). pool_size=0 keeps that legacy default; >0
overrides via ConnectionPool::set_max_idle_per_key. The C++ pool
has no hard ceiling on total concurrent clients per key — only this
idle-queue cap — so the semantics differ slightly from Go's
PoolSize ceiling. The schema is symmetric across engines; the
effective bound is the engine's strictest one.
Test:
- "redis_connection schema exposes cascade-safety defaults" locks
the cross-engine-shared defaults (2000 ms x 4, pool_size=0) on
the registered schema, matching pine-go's
TestRedisOptionsFromParams_Defaults and pine-java's
schemaExposesCascadeSafetyParams.
- 214/214 pine_cpp_tests still green.
Refs: 2026-06-22 incident postmortem (recsys for_you).
…eout schema
Picks up the dial_timeout_ms / read_timeout_ms / write_timeout_ms /
pool_timeout_ms / pool_size params added to the redis_connection
resource schema across pine-go / pine-java / pine-cpp.
The Python DSL now accepts these as kwargs on RedisConnectionResource:
flow.resource("redis_conn", RedisConnectionResource(
addr=_redis["host"],
password=_redis.get("password", ""),
metrics_name="recsys",
read_timeout_ms=500, # cap any single redis call at 500ms
write_timeout_ms=500,
dial_timeout_ms=1000,
pool_timeout_ms=1000,
pool_size=50,
))
Generated by `scripts/codegen.sh` (Go backend); cross-validate is green
across all three engines.
Refs: 2026-06-22 incident postmortem (recsys for_you).
… default literal)
Three rendering drifts surfaced when round-tripping the redis_connection
schema change through scripts/codegen.sh --backend java:
Drift Go (canonical) Java (pre-fix)
─────────────────────────────── ────────────────── ────────────────
**Type** heading `Transform` `transform`
String default in param table `` `"string"` `` `` `string` ``
Bool default in param table `` `False` `` `` `false` ``
Root causes:
- fromRegistry() used `enum.name().toLowerCase()`, producing all-lowercase
("transform") whereas Go's OperatorType constants are PascalCase
("Transform"). Replaced with pascalCaseEnum() that splits on "_" so
future multi-word types (e.g. MERGE_DEDUP_UNION → MergeDedupUnion)
render cleanly.
- Per-operator markdown was rendering `spec.defaultValue` via implicit
toString(), bypassing the existing toPythonLiteral() helper. Bare
toString() produces "string" (no quotes) and "false" (lowercase),
whereas Go's pythonLiteral uses fmt.Sprintf("%q") for strings and
emits "True"/"False" for bools. Wired toPythonLiteral() into the
doc render path so a single helper now governs both Python code
and markdown rendering.
Why nobody saw this before:
- Production codegen runs use the Go backend (default of codegen.sh).
- cross-validate's 01-codegen-schema.sh checks structural schema
parity (param types/required/defaults) and byte-level Python
output parity, but never compared the generated markdown across
engines, so the Java backend's drift sat dormant.
Why pine-cpp is fine:
- pine-cpp's pineapple-codegen has no markdown emit path (only
--schema-json export and Python codegen). The Python emit's
python_literal() already mirrors Go (True/False, string-quoted),
and 01-codegen-schema.sh's `Go vs C++ Python output (byte-level
match)` check has been guarding this since it was added.
Test:
CodegenDocParityTest pins:
- pascalCaseEnum on each registered OperatorType + multi-word
defensive case + null/empty edge cases.
- toPythonLiteral on string (quoted), bool (capitalised),
integer (no decimal), null (None).
243/243 pine-java tests green (was 236, +7 from the new test).
cross-validate all green (no real FAILs).
Refs: 2026-06-22 incident postmortem (recsys for_you).
…ected()=false
Two fixes around Client construction so Redis flakiness stays at the operator
boundary (fail_on_error=false → cache miss + warning) instead of escalating
into a request abort or process abort.
(1) AUTH / SELECT failure now sets connected()=false instead of throwing
Pre-fix the Client ctor would let std::runtime_error from expect_ok() unwind
out of std::make_unique<Client> in ConnectionPool::acquire and tear down
the request. The dial-timeout / connect-failure paths above already silently
fall through to fd_ = -1, so AUTH/SELECT was the asymmetric path. The 2026-
06-22 review (.code-review/from-24975c2/...) flagged this as the contract
mismatch with the Redis section of llmdoc/reference/operator-contract.md.
Fix: wrap AUTH/SELECT in try/catch, close fd_ on exception, leave the rest
of the protocol-error throws in place (PING / read_line / read_bulk_string
etc.) — those are during normal command execution and the operator already
catches them.
(2) send_command now uses send(MSG_NOSIGNAL) instead of write()
A Redis server that closes the connection mid-write (RST / transient broker
restart) would deliver SIGPIPE to the process. The cascade-safety contract
for this resource is that Redis flakiness must never escalate beyond the
operator. SIGPIPE'ing the whole pineapple-server is the antithesis of that;
the change turns the dropped peer into a normal throw, which (1) above and
the operator-level fail_on_error=false branch surface as a warning.
Surfaced while writing the regression test for (1) — the test server closes
its accept'd socket immediately, the client's AUTH write hits a closed peer,
and pre-fix the test process aborted with SIGPIPE before it could even check
connected(). pine-cpp/src/server/server.cpp:461 already used MSG_NOSIGNAL
for the same reason; the redis client was the missing peer.
(3) Documentation: connect_with_timeout's failure contract
Made explicit in the function-level comment that on false the caller MUST
close the fd and that the socket may still be in non-blocking mode. Today
the sole caller in this file does close fd on false (the only safe pattern),
but the contract was implicit; pinning it heads off future drift.
Test:
- redis Client AUTH/SELECT failure marks connected=false instead of
throwing — binds a 127.0.0.1 ephemeral listener, accepts then closes
immediately, constructs Client with non-empty password (forcing AUTH).
Pre-fix the ctor threw out of the test (verified by temporarily
disabling the try/catch — got SIGABRT). Post-fix connected()==false
and the test joins cleanly.
- 215/215 pine_cpp_tests green.
Refs: 2026-06-22 incident postmortem (recsys for_you), code review
.code-review/from-24975c2/from-24975c2-to-77b6e18.md.
…ote in schema
The 2026-06-22 review (.code-review/from-24975c2/...) flagged that pine-java's
single-socket-timeout mapping is asymmetric with Go and C++:
Go / C++: read_timeout and write_timeout honoured independently
(separate SO_RCVTIMEO / SO_SNDTIMEO; redis.Options).
Java: Jedis exposes only socketTimeoutMillis. Currently mapped as
max(read_timeout_ms, write_timeout_ms) so reads that wait on
the server (LRANGE / ZRANGEBYSCORE) aren't capped at write_timeout.
Two issues with the original:
(1) The trade-off was buried in a comment at the call site only — users
looking at the schema description (or the Apple Python codegen-emitted
docstring) would never know. Configuring read_timeout_ms=500,
write_timeout_ms=2000 produces a 500 ms wall on Go but 2000 ms on
Java, which is the exact "surprise" cascade-safety is meant to prevent.
(2) The policy choice (max vs min) was a private detail with no test
coverage. A future refactor could flip it to min — which would silently
turn long-running reads into write-timeout failures — and only
production traffic would catch the drift.
Fixes:
- Extract the mapping into AllOperators.effectiveSocketTimeoutMs(read, write),
package-private, with a Javadoc that explains why max wins.
- Update the write_timeout_ms schema description on all three engines
(Go / Java / C++) to surface the Java-specific note. Apple Python codegen
reads the Go schema by default, so the Python docstring now carries the
warning. The note also lands in pine-cpp's redis_connection.cpp comment
block at the top of the file for consistency.
- New regression tests on RedisConnResourceTest:
* writeTimeoutMapsToMaxOfReadAndWriteOnJedis — pins the helper output
on symmetric / asymmetric / default inputs.
* writeTimeoutSchemaDescriptionMentionsMaxBehaviour — pins the
description language so a future drift on policy is visible at
config-time.
6/6 RedisConnResourceTest green; 245/245 pine-java tests overall.
cross-validate green (no real FAILs).
Refs: 2026-06-22 incident postmortem (recsys for_you), code review
.code-review/from-24975c2/from-24975c2-to-77b6e18.md.
…o doc drifts
The 2026-06-22 review (.code-review/from-24975c2/...) flagged three drifts
between Go and Java markdown emitters and a deeper UX paper-cut: alphabetical
param ordering (`addr, db, dial_timeout_ms, metrics_name, password, ...`)
visually mixed mandatory and optional knobs together. With the new five
Redis-cascade params the chaos is acute; this commit aligns the three engines
on a tier-then-alphabet ordering.
Changes:
(1) sortedParams now returns required-first, then optional, alphabetised
within each tier. pine-go/pkg/codegen/template.go and pine-java's
Codegen.java grow a shared sortedParams helper; pine-cpp's
cmd/pineapple-codegen/main.cpp gets sorted_param_names with the same
semantics. operators.py and resources.py now lead with required keys
(e.g. lua_script, host+port, key_prefix+resource_name) — readers see
the must-supply contract first. The byte-level Python output stays
comparable across all three engines (cross-validate 1c/1d gates green).
(2) pine-java Codegen no longer drops absent Metadata Contract rows. Pre-fix
operators that declared only ItemOutput (e.g. recall_resource) emitted
a single-row table; pine-go's BacktickWrap renders empty fields as "-"
so all four rows are always present. Java's markdown now renders the
same shape via a small backtickOrDash helper.
(3) scripts/codegen.sh --backend java now passes -ops-dir to Codegen so the
javadoc-driven Metadata Contract parsing runs. Pre-fix the path was
dormant — Java markdown silently omitted every Metadata Contract section
because metadataDocs was always empty.
(4) Schema description string fixes in pine-java/AllOperators.java:
transform_copy.direction, merge_dedup.strategy, reorder_sort.order,
transform_redis_{get,set}.data_type, observe_log description. All
carried subtly different language from pine-go (missing surrounding
quotes around enum values, abbreviated observe_log description).
Aligning them is what made the Go-vs-Java doc diff converge.
(5) README.md trailing whitespace: pine-java now emits the same blank-line
boundary as pine-go's index template (one blank between description and
first category, trailing newline at EOF).
(6) llmdoc/reference/operator-contract.md Redis section now lists all
eleven redis_connection params (was six pre-PR), surfaces the
pine-java max(read,write) note, and flags the pine-cpp pool_timeout_ms
no-op + pool_size semantic difference. Also adds the cascade-safety
background sentence pointing back to the 2026-06-22 incident.
What's intentionally NOT in this commit:
- pine-cpp doc emit. cpp's pineapple-codegen has no markdown path today and
cannot synthesize the Metadata Contract section without source-comment
parsing or a schema-side MetadataDoc field. The follow-up commit lifts
metadata onto OperatorSchema directly so all three engines render from
a single source of truth, then cross-validate gains a Go-Java-cpp
markdown byte-equal gate.
- Go-Java markdown byte-equal gate in cross-validate. Will land in the
same follow-up alongside pine-cpp doc emit so the gate covers all
three engines symmetrically.
Test:
- go vet + go test ./operators/transform/ -run TestRedis green
- mvn test green (245 tests; +2 RedisConnResourceTest cases for the
write_timeout policy lock from the previous commit)
- pine_cpp_tests 215/215 green
- cross-validate green (no real FAILs); 01-codegen-schema 1a-1d
(schema parity Go-Java-cpp + Python output byte-equal Go-Java-cpp)
all PASS.
Refs: 2026-06-22 incident postmortem (recsys for_you), code review
.code-review/from-24975c2/from-24975c2-to-77b6e18.md.
…-schema gate Closes the dormant gap surfaced by the 2026-06-22 review: 01-codegen-schema.sh has long checked schema structure parity (1a/1b) and Python output byte-equal parity (1c/1d), but never compared the generated markdown across engines. Three doc-rendering drifts had been sitting under that blind spot — see commit 75ce3c0 (the required-first ordering + Java/Go doc fixes batch). Adds section 1e: run pine-go and pine-java codegen with -doc-dir to a scratch directory each, then `diff -r` the two trees. Fails the section on any byte-level disagreement and dumps the diff to stderr so future drifts surface in CI rather than at the next review. Two ancillary fixes were needed to make the gate actually pass: (1) pine-java's `if (md != null)` guard around the Metadata Contract section let operators without a parsed javadoc Metadata block (transform_bench_cpu / transform_bench_sleep) silently omit the section; pine-go's template emits it unconditionally with all four rows reduced to "-". Java now mirrors that — synthesise an empty MetadataDoc when the parser found nothing, then render the same four rows. (2) pine-java's TransformCopy javadoc was missing the "(collects all item values into a list)" annotation on the item_to_common direction's CommonOutput row. pine-go's source comment had it; Java's parser dropped the line, so its rendered table cell trailed Go's by one phrase. (3) Schema description fixup: redis_set's fail_on_error description was the wrong wording on Java side ("logging to stderr" instead of pine-go's "logging and continuing"). Aligned. pine-cpp has no markdown emit path today, so the gate covers Go-Java only. Adding the Go-cpp arm is on the follow-up: it requires lifting metadata onto OperatorEntry (cpp has no source-comment parser) and porting the doc template to cpp. The schema-side groundwork is in place — types.OperatorSchema would learn a Metadata field — but the 22-operator migration belongs in its own commit so this gate can land clean. Test: - cross-validate green (no real FAILs); section 1e reports "PASS: codegen Markdown output parity Go vs Java (byte-level match)". - 245/245 pine-java tests still green. - Sanity-checked the gate itself by temporarily reverting the backtickOrDash change: the gate FAILed with a recall_resource Metadata Contract divergence, exactly the class of regression we want it to catch. Refs: 2026-06-22 incident postmortem (recsys for_you), code review .code-review/from-24975c2/from-24975c2-to-77b6e18.md.
The 2026-06-22 review (.code-review/from-24975c2/...) recommended
gating Go-Java markdown byte-equality in cross-validate; the previous
commit landed that gate. Following through on the request to extend
the gate to pine-cpp ("不要后退, 不要打折扣"): pine-cpp's
pineapple-codegen now has a markdown emit path of its own, the gate
covers all three engines symmetrically, and the schema-side change
keeps metadata authoring in lockstep with how Go and Java work.
Changes:
(1) OperatorSchema gains an optional MetadataDoc field
pine-cpp/include/pine/operator.hpp adds MetadataDoc + a
schema.metadata field. Empty fields render as "-" in the generated
markdown (matches BacktickWrap on the Go side and backtickOrDash on
the Java side). pine-cpp has no source-comment parser like
pine-go's pkg/codegen/docparse.go or pine-java's
parseOperatorDocs, so cpp operators must declare metadata inline at
registration time. The cross-engine codegen contract is now
consistent: Go/Java parse from comments/javadoc, cpp declares
inline; all three feed the same renderer shape.
(2) 19 cpp operators populate .metadata to match pine-go's
source-comment metadata verbatim
filter_{condition,paginate,truncate}, merge_dedup, observe_log,
recall_{resource,static}, reorder_{shuffle_by_salt,sort},
transform_{by_lua,by_remote_pineapple,copy,dispatch,normalize,
redis_get,redis_set,resource_lookup,size}, transform_bench_{cpu,
sleep}. The strings are extracted from the rendered Go markdown
(so e.g. transform_copy's CommonOutput inherits the
"(collects all item values into a list)" annotation that Go's
metadata-section parsing collected from the multi-direction
comment block).
(3) pineapple-codegen learns a -doc-dir flag
pine-cpp/cmd/pineapple-codegen/main.cpp grows write_operator_doc_md
+ write_doc_index_md + an op_type_pascal helper that mirrors
pine-go's `string(schema.Type)` (PascalCase) and pine-java's
pascalCaseEnum. The output is byte-equal with pine-go's emit when
every operator declares matching metadata.
(4) cross-validate gate extends to Go vs cpp
scripts/cross-validate/01-codegen-schema.sh's section 1e gains a
Go-vs-cpp arm symmetric with the existing Go-vs-Java arm. The cpp
arm only runs when CPP_CODEGEN is set (i.e. the cpp binary is
built), matching the existing skip-when-absent pattern in 1d.
Test:
- 215/215 pine_cpp_tests still green.
- cross-validate green; section 1e reports both
"PASS: codegen Markdown output parity Go vs Java" and
"PASS: codegen Markdown output parity Go vs C++".
- Sanity: diff -r doc/operators against pineapple-codegen
-doc-dir output produces zero diffs.
Refs: 2026-06-22 incident postmortem (recsys for_you), code review
.code-review/from-24975c2/from-24975c2-to-77b6e18.md.
Adds two new metrics on every redis_connection resource (gated by the
existing metrics_name knob, like the pool gauges and PING probe):
pine_redis_command_duration_seconds histogram labels: name, command, status
pine_redis_command_total counter labels: name, command, status
Status taxonomy (identical across pine-go / pine-java / pine-cpp):
ok — call returned (redis.Nil cache miss is not an error)
timeout — context.DeadlineExceeded or net.Error.Timeout()
pool_timeout — connection-pool wait deadline exhausted
error — anything else (connection reset, AUTH, protocol)
Why now
-------
The 2026-06-22 recsys outage's only signals were the PING probe
(sampled every 15s) and the pool gauges. Neither distinguishes "Redis
is slow" from "we ran out of pool" from "Redis up but rejecting"; the
review (.code-review/from-24975c2/...) flagged the gap. With these
metrics the per-second timeout rate is queryable, and a Grafana panel
on rate(pine_redis_command_total{status="timeout"}[1m]) will fire
during the next incident's first 5s of deviation rather than after
the dashboard reader correlates pool usage by hand.
Implementation per engine
-------------------------
pine-go: redis.Hook on the *redis.Client wraps every command;
recordCommand classifies the error and emits both metrics. The hook
filters HELLO / CLIENT / PING / AUTH / SELECT (lifecycle + probe
traffic) so the metric reflects business commands only — pine-java
and pine-cpp instrument at the operator-level facade and never see
those, so filtering Go keeps the three engines emitting comparable
cells under the same workload (cross-validate 16 asserts cell-set
parity).
pine-java: Jedis has no hook interface, so RedisConnResource grows
runCommand(command, action). transform_redis_get / transform_redis_set
route every call through it; the pool.getResource try-with-resources
moves into the helper. redisCommandStatus walks the JedisException
cause chain (SocketTimeoutException → timeout) and recognises the
JedisException("Could not get a resource") pool-exhaustion signature.
pine-cpp: same shape as Java — RedisConnResource::run_command<T>(name,
fn) template. The cpp client throws std::runtime_error for every
failure mode, so timeout / pool_timeout aren't currently distinguished
(all non-success lands under "error"); future work is to make Client
throw distinct types so cpp can match Go's classifier.
Tests
-----
pine-go (TestRedisCommand*): classifier table covering all 7 status
cases, end-to-end miniredis fixture asserting label tuples on SET /
GET / cache-miss, gating test for empty metrics_name, filter test
proving HELLO / CLIENT / PING are elided while SET is recorded.
pine-java (RedisConnResourceTest.commandStatusClassification): pins
the JedisException -> status mapping with sentinel exceptions for
each branch (pool timeout, socket timeout, connection refused,
data error, generic).
pine-cpp (test_redis_resource.cpp kRedisMetrics): adds the two new
metric names to the gate list — both must register when metrics_name
is set, neither when it's empty.
cross-validate (section 16): metric-name list expanded to 6; per-
command cells are stripped from the byte-equal shape diff because
Java/cpp instrument at the operator boundary while go-redis surfaces
all command traffic, so the cells legitimately differ outside the
business-command intersection. Names + correctness still parity-
gated.
Operator wiring on tipsy-recsys's own redis ops
(transform_redis_zrangebyscore, transform_query_blocked_creators) is
out of scope for this PR — the upstream resource emits the metrics,
but the recsys-side operator instrumentation will land in a recsys
follow-up.
Refs: 2026-06-22 incident postmortem (recsys for_you), code review
.code-review/from-24975c2/from-24975c2-to-77b6e18.md.
(1) pine-cpp run_command no longer prefixes the dial-failure message
with "redis: ". The pre-refactor cpp ops produced the warning
"transform_redis_get: Get(<key>): connection failed"; the refactor
accidentally drifted to "...: redis: connection failed" because
the std::runtime_error thrown from run_command embedded its own
prefix, then the operator's on_failure(e.what()) wrapped it again.
The 2026-06-23 increment-2 review (.code-review/from-24975c2/...)
flagged this as a minor wording shift not under byte-exact
cross-engine parity but worth restoring.
(2) Removed pine-java TransformRedisGet.borrowPool. The runCommand
refactor switched both internal callers (TransformRedisGet,
TransformRedisSet) to borrowResource(...) directly, leaving
borrowPool a one-line wrapper with zero callers. CLAUDE.md's
project convention is to avoid backwards-compat shims while
in-development; the same review noted it as dead code.
Test:
- 215/215 pine_cpp_tests green.
- 246/246 pine-java tests green.
- cross-validate green (no real FAILs).
Refs: 2026-06-23 review increment-2-to-4a3bf3e.md, code review
.code-review/from-24975c2/.
… + codegen parity Reflection: llmdoc/memory/reflections/redis-cascade-safety-and-observability.md Stable doc updates promoted from the reflection's 5 candidates: - must/conventions.md: codegen single-direction alignment (Go is source of truth; pine-java / pine-cpp align to Go; do not push fields onto Go schema to make Java align). pine-cpp raw socket clients must use MSG_NOSIGNAL (the redis client missed it; surfaced via the AUTH-failure test fixture). - reference/operator-contract.md: failed-path silent-degrade audit contract — every new client/resource failure path (AUTH/SELECT/dial/ pool) must walk the fail_on_error=false → connected()==false → silent degrade chain end-to-end. Redis chapter's metrics_name now points at both pool/probe and per-command metric groups. - reference/metrics-observability.md: per-command Redis metrics (pine_redis_command_duration_seconds + pine_redis_command_total), status taxonomy (ok / timeout / pool_timeout / error), lifecycle- command filtering (HELLO / CLIENT / PING / AUTH / SELECT) on the Go hook so cells stay byte-comparable across engines whose facades sit at different layers, and the cpp known follow-up (single std::runtime_error type collapses timeout/pool_timeout into error). - guides/cross-layer-validation.md: codegen markdown byte-equal gate as the final guard (unit tests pinning a single helper aren't enough); 01-codegen-schema.sh's section 1d/1e covers Go-vs-Java + Go-vs-cpp. - guides/standard-workflow.md: review-driven scope expansion is OK — don't default to compromise. Per-command observability landed because the first review round flagged it; the cleanest implementation cost ~1.5h more than the compromise variant. - architecture/pine-cpp-runtime.md: pineapple-cpp-codegen -doc-dir markdown emit + OperatorSchema.metadata field; redis Client AUTH/ SELECT failure containment + send(MSG_NOSIGNAL); run_command<T> template + error-type stratification known follow-up. - index.md: indexed the new reflection, adjusted descriptions to point at new sections. No quantitative numbers hard-coded (count of metrics / operators / cross-validate sections kept symbolic); follows the project convention on numeric-description avoidance.
…gProvider Pre-push lint flagged QF1008: recordingProvider embeds metricRecorder so r.record(...) resolves the same way as r.metricRecorder.record(...) but without the redundant selector. No behaviour change. Caught by .githooks/pre-push (golangci-lint staticcheck).
Contributor
🔍 PR 审查
代码质量高，三主题（cascade-safety 五参数 / cpp Client 失败收敛+SIGPIPE / codegen parity+per-command metrics）逻辑正确、三引擎对齐严谨、注释充分、测试与 cross-validate gate 完备。无阻塞问题。已逐文件核对核心逻辑（Go reference 实现、Java facade、cpp run_command 模板、codegen 渲染、connection_pool 改造）与版本同步（0.10.10 已在 Go/Java/cpp kVersion/apple 四处一致）。仅 2 个不影响行为的小问题。
|
The agentic-pr-review on PR #137 flagged that redisCommandStatus was classifying context.Canceled as `timeout`, which violates metrics-observability.md's status taxonomy. The taxonomy reserves `timeout` for read/write_timeout_ms expirations (i.e. "Redis is slow") so that rate(pine_redis_command_total{status="timeout"}[1m]) reflects upstream pressure on the Redis path. context.Canceled means the caller (HTTP handler / outer ctx) gave up — that's a request-lifecycle event, not a Redis health signal — so a panel querying status="timeout" should NOT include it. Reclassifying it as `error` keeps the timeout bucket monosemantic. Updated TestRedisCommandStatus/context_canceled to expect "error". Inline comment pins the taxonomy decision so a future refactor doesn't re-merge them. Refs: 2026-06-23 PR #137 agentic-pr-review.
…eference The agentic-pr-review on PR #137 flagged that the write_timeout_ms schema description text was drifting across the three engines: pine-go and pine-cpp ended on "pine-go and pine-cpp honour read/write independently." while pine-java's truncated before that sentence. Behaviourally inert — apple_generated/resources.py is generated from pine-go (the source of truth), so users see the Go wording verbatim. The drift was only visible when auditing each engine's internal schema export. Aligning the three sources avoids future audit noise and keeps the cross-engine description registry comparable byte-for- byte for the next time someone runs `Codegen --export-schema` against Java or `pineapple-codegen -schema-json` against cpp. cross-validate's section 1e markdown gate covers `doc/operators/` only — resource schema descriptions are not part of that gate, so this drift slipped through CI; reviewer correctly identified the gap. Refs: 2026-06-23 PR #137 agentic-pr-review.
Contributor
🔍 PR 增量审查
本轮增量仅两个 commit，恰好闭环上次审查（截止
|
Liam0205
added a commit
that referenced
this pull request
Jun 25, 2026
…fan-out Core features list was last refreshed pre-v0.10; this batch syncs to v0.10.9 reality: - Lua: explicit pine-go default = wangshu (with build-tag escape to gopher-lua), pine-java = LuaJC, pine-cpp = LuaJIT. The default flip landed in v0.10 series. - Resources: split into data-typed (snapshot) vs handle-typed (borrow + RAII teardown) — `redis_connection` is the canonical handle-typed resource. The old one-liner "background-refreshed in-memory resource manager" hides the architecture. - Redis: add dedicated bullet for the 5 cascade-safety params and the 4-state per-command metrics + fail-on-error contract. These shipped in #137 and are production-load-bearing. - Observability: /stats is no longer just a single endpoint — call out /stats.http and /stats.resources sub-trees so readers can find the resource fan-out and HTTP middleware metrics. - Cross-validation bullet: name the actual verification surface (19 cross-validate sections + differential fuzz + daily sanitized fuzz) instead of the vague "verified for schema/DAG/exec/error parity". EN side kept structurally aligned with the CN edits.
Liam0205
added a commit
that referenced
this pull request
Jun 25, 2026
Re-ran the full 14-fixture cross-runtime bench on the standard 2C/4G cgroup (10000 req × 16 conc) on the same machine that produced the previous 2026-06-11 numbers. v0.10 series picked up wangshu CallInto / GlobalsSlot fast paths, outputPool (#119), and Redis cascade-safety (#137) — re-baseline so the README reflects measured reality. Changes worth calling out: - Three calibrated fixtures now listed instead of one. Until now the README collapsed the calibrated family to a single row, hiding the itemlua variant entirely. itemlua (3000 Lua calls/request) is the boundary-dominated workload that anchors the perf-evolution-roadmap "calibration fact 2 — end-to-end dilution" finding; it deserves to show up. - C++ headline lift: 1.8x → 1.9x against Go/Java on calibrated. P50 60.8ms vs 117/122ms is the more legible framing than the QPS ratio. - Synthetic small/medium movements are all within ±10 % run-to-run noise; the relative shape (Go highest at small, Java reverses on large_1000+) is unchanged. - Reproduce command now lists `make bench-cross-runtime` first. Source data: bench-results/report-20260625-090834.txt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
背景
2026-06-22 tipsy-recsys redis 雪崩故障的工程修复。Redis PING p99 短期飙至 ~970ms，pre-fix 资源沿用 client 默认值（go-redis v9: read/write 3s, dial 5s, PoolSize 20）使每个 /execute 请求阻塞 in-flight、heap_inuse 单 pod 飙至 3.87 GiB、运行时 OOM。
主线变更（三主题）
A. Redis cascade-safety 五参数暴露（三引擎对齐）
redis_connection资源新增：dial_timeout_ms(默认 2000)read_timeout_ms(默认 2000) — 雪崩防护核心write_timeout_ms(默认 2000)pool_timeout_ms(默认 2000)pool_size(默认 0 = 引擎默认)引擎注释：
max(read, write)；schema description 明确点出pool_timeout_ms当前 no-op（acquire 不阻塞，按 idle queue cap 工作），保留 schema 对称生产部署典型：
read=500, write=500, dial=1000, pool=1000, pool_size=50B. pine-cpp Client 失败收敛 + SIGPIPE 守卫
connect_with_timeout失败路径契约文档化（调用方 close fd + socket 可能仍 nonblocking）write()→ redis 中段断连会 SIGPIPE 整死进程！pine-cpp/server.cpp 早就用 MSG_NOSIGNAL，redis client 漏了。生产 bug。改用send(MSG_NOSIGNAL)。C. Codegen markdown 跨引擎 byte-equal + per-command observability
Codegen markdown parity：
OperatorSchema.metadata字段（22 cpp 算子 inline 声明）01-codegen-schema.sh加 section 1e（Go-vs-Java + Go-vs-cpp doc byte-equal gate）per-command Redis metrics：
pine_redis_command_duration_seconds（histogram）+pine_redis_command_total（counter）name/command/statusok（含 redis.Nil cache miss）/timeout/pool_timeout/errorHELLO/CLIENT/PING/AUTH/SELECT（lifecycle/probe）让 cells 跨引擎对称runCommand(name, action)facaderun_command<T>(name, fn)；cpp client 单一 std::runtime_error 类型，timeout/pool_timeout 当前落 error 桶（known follow-up：client 错误类型分层）D. llmdoc 同步
reflection:
redis-cascade-safety-and-observability.md5 条 promotion 落入：
must/conventions.md: codegen 单向对齐（Go 是 source of truth）+ pine-cpp 网络客户端 MSG_NOSIGNAL 契约reference/operator-contract.md: failed-path 静默降级审计契约reference/metrics-observability.md: per-command 指标 + status taxonomy + lifecycle 过滤guides/cross-layer-validation.md: codegen markdown byte-equal gate 检查项guides/standard-workflow.md: review-driven scope expansion 接受architecture/pine-cpp-runtime.md: doc emit + Client 失败收敛/MSG_NOSIGNAL + run_command测试
TestRedisCommand*9 个新测试（classifier 7 case + miniredis E2E + 过滤器）；TestRedisOptionsFromParams 3 case；全 PASScommandStatusClassification5 case + write_timeout policy/description 双锁定）三轮 code review（本地）
from-24975c2-to-77b6e18.md: REQUEST_CHANGES（1 阻塞 + 4 重要 + 2 小，全部闭环）increment-1-to-6c6478b.md: APPROVEincrement-2-to-4a3bf3e.md: APPROVE（2 小问题已修）业务方观测
事故后业务可建 Grafana 报警：
```promql
rate(pine_redis_command_total{status="timeout"}[1m]) > 0
rate(pine_redis_command_total{status="pool_timeout"}[1m]) > 0
histogram_quantile(0.99, rate(pine_redis_command_duration_seconds[1m]))
```
Follow-up
transform_redis_zrangebyscore/transform_query_blocked_creators）接 metric 在 recsys repo 单独 PRRefs: 2026-06-22 incident postmortem (recsys for_you).