Skip to content

Commit 9b5f6d4

Browse files
learn: retrospective learnings
Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
1 parent 2072b25 commit 9b5f6d4

1 file changed

Lines changed: 8 additions & 0 deletions

File tree

‎.claude/knowledge/learning-log.md‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -140,4 +140,12 @@ until a human merges it. The engineer author phase reads this log (see
140140
- **Context:** PR #975 bumped thrift from 0.24.x to 0.25.0 to clear CVEs; 0.25.0's CVE-2026-85494 fix changed `TBinaryProtocol`'s default `string_length_limit`/`container_length_limit` from unbounded (None) to ~15.6 MiB. The connector reads inline Arrow result batches that routinely exceed that, so it had to pass `string_length_limit=None, container_length_limit=None` explicitly (with a regression test asserting the kwargs) to preserve its pre-upgrade, already-`buffer_size_bytes`-bounded behavior.
141141
**Rule:** When bumping a dependency — especially a security release — check whether it introduces or tightens a default limit/bound (max size, recursion depth, buffer cap); if your code relies on the prior behavior, set that parameter explicitly rather than inheriting the new default, and add a test asserting the explicit value so a future bump can't silently cap or regress it.
142142

143+
### 2026-10-13: learnings since 2026-10-12T17:33:54Z
144+
- **Context:** PR #974 replaced the telemetry flag gate `str(flag_value).lower() == "true"` with a strict `get_bool` that requires `json.loads(raw)` to yield a bare JSON boolean; reviewer noted server values like `True`/`TRUE` or a JSON-quoted `"true"` now silently fall through to the `False` default, and the unit mocks (which store the bare lowercase form) hid the regression.
145+
**Rule:** When tightening a lenient value parser into a strict typed one, confirm the exact wire format the server actually emits and add tests covering the non-canonical forms (uppercase, quoted, numeric-vs-bool) — otherwise previously-tolerated values silently regress to the default.
146+
- **Context:** PR #974 moved the feature-flag lock from per-connection to a shared per-workspace `_CacheState.lock`; reviewer flagged that the first flag read runs a synchronous `request(..., timeout=30)` inside `with self._state.lock`, so one connection's initial fetch can block every other connection to that workspace for up to 30s.
147+
**Rule:** Do not hold a shared lock across blocking network I/O — dedupe/coordinate the fetch without serializing independent callers on the slow request (e.g. release the lock during I/O or use an in-flight Future), especially when a lock's scope was just widened from per-connection to shared.
148+
- **Context:** PR #974 had each `FeatureFlagsContext` capture `self._executor` at construction while `FeatureFlagsContextFactory.remove_instance` can `shutdown()` and null out that executor; a context outliving a remove→get cycle would `submit()` onto the dead executor, raising `RuntimeError: cannot schedule new futures after shutdown` outside the guarding try/except.
149+
**Rule:** Don't cache a reference to a shared singleton resource (executor, client, pool) that another code path can tear down and recreate — re-read it from the factory at use time, or guard the use against the post-shutdown error.
150+
143151
--- *Add new entries above this line (oldest→newest); newest sections sort to the bottom.* ---

0 commit comments

Comments
 (0)