Skip to content

Commit 543544f

Browse files
learn: retrospective learnings
Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
1 parent 50d4166 commit 543544f

1 file changed

Lines changed: 12 additions & 0 deletions

File tree

.claude/knowledge/learning-log.md

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,3 +93,15 @@ initial dated section below.
9393
### 2026-09-09: learnings since 2026-09-08T17:27:48Z
9494
- **Context:** PR #522 bumped vulnerable dev dependencies by regenerating package-lock.json, but the refreshed js-yaml entries (3.15.2 and 4.3.2, in every nested copy) had `resolved` URLs pointing at the internal `npm-proxy.cloud.databricks.com` registry instead of the public `registry.npmjs.org`; a reviewer caught that this breaks `npm ci` for external contributors since the Databricks proxy host is not publicly reachable.
9595
**Rule:** When regenerating a lockfile for a public/open-source package, verify every `resolved` URL points to the public `registry.npmjs.org` (not an internal proxy like `npm-proxy.cloud.databricks.com`); `integrity` hashes are content-addressed and match either way, but a non-public host silently breaks `npm ci` for outside contributors.
96+
97+
### 2026-09-12: learnings since 2026-09-11T17:27:23Z
98+
- **Context:** In PR #523, `StatusError` (lib/errors/StatusError.ts) `implements Error` but is NOT a subclass of `Error`; the Reyden fallback matched on `error instanceof StatusError && error.sqlState === 'KP001'` and had to re-throw the original object unchanged on non-matching errors.
99+
**Rule:** Classes that `implements Error` without extending it still satisfy `instanceof` for that class but are not real Errors — re-throw them as-is; never normalize/wrap them through generic Error handling or custom fields like `sqlState`/`message` are lost.
100+
- **Context:** PR #523 review caught that `KernelBackend.connect()` installs a process-global log-bridge listener (via `logger.onLevelChange`) released only by `KernelBackend.close()`; constructing a fresh fallback KernelBackend per `openSession` leaked one listener per call because `ThriftBackend.close()` never closed them. Fix memoized a single fallback backend and closed it in `close()`.
101+
**Rule:** A backend/resource whose `connect()` registers a process-global listener must be created once per connection (memoized) and explicitly closed in the owner's `close()`; never construct it per-request or listeners accumulate for the process lifetime.
102+
- **Context:** PR #523 review found a close-during-connect race: `close()` released the fallback KernelBackend via a field assigned *inside* an async connect IIFE (after `await connect()`), so a `close()` racing an unresolved connect skipped it and the late-resolving connect orphaned a backend that had installed a process-global listener. Fix awaited the in-flight connect promise in `close()`.
103+
**Rule:** When a resource is torn down via state set inside an in-flight async init, have `close()` await the pending init promise (the single source of truth) and close whatever it produces — reading a not-yet-assigned field orphans resources initialized after close() runs.
104+
- **Context:** In PR #523 reviewers noted the Node driver never inspected SQLSTATE before, and asked why cache expired-key sweeping wasn't done "like Python and Go"; the fix added a `markReyden` sweep to match the sibling drivers.
105+
**Rule:** This SQL driver is one of several parallel implementations (Python, Go, Node); when adding cross-cutting behavior (SQLSTATE handling, cache eviction, fallback routing) check the sibling drivers and match their semantics rather than inventing divergent behavior.
106+
- **Context:** PR #523 review flagged the cache's 6h TTL expiry/eviction branch as untested because `Date.now()` wasn't injectable; the fix added coverage with sinon fake timers, asserting both the strict-`>` TTL boundary and the sweep-on-mark path (proven via `size()` without a lookup that would trigger lazy eviction).
107+
**Rule:** Time-based cache expiry/eviction must be tested with a fake clock (e.g. sinon `useFakeTimers`), asserting the exact TTL boundary and any proactive sweep independently of lazy per-access eviction — a suite that never advances time silently hides TTL regressions.

0 commit comments

Comments
 (0)