Skip to content

Renew processing locks and surface handler errors - #15

Draft
mayf3 wants to merge 3 commits into
larksuite:mainfrom
mayf3:agent/renew-processing-lock-and-surface-handler-errors
Draft

Renew processing locks and surface handler errors#15
mayf3 wants to merge 3 commits into
larksuite:mainfrom
mayf3:agent/renew-processing-lock-and-surface-handler-errors

Conversation

@mayf3

@mayf3 mayf3 commented Aug 19, 2026

Copy link
Copy Markdown

Independent review amendment V2

Review identity: CODEX_LARK_CHANNEL_SDK_PR15_INDEPENDENT_REVIEW_2026_08_19

The reviewed source head 3940d8a67709902584f040fea1b4c2aa0418f135 received
REQUEST_CHANGES. Corrective commit bd24f6742513769c80b5401b96ad464d74dd2027
addresses all three blockers without rewriting the reviewed history:

  1. Lease ownership: ProcessingLock.acquire now returns an immutable,
    tokenized ProcessingLease. Exact leases flow through batching/queueing and
    are required by stopRenewal and release; stale owners are no-ops. Active
    and finalizing leases cannot be stolen on wall-clock expiry.
  2. Async error observers: the public error handler may return a Promise.
    Sync throws, rejected Promises, and throwing thenables are consumed and
    logged without recursive emission, unhandled rejection, or cleanup impact.
  3. Duration validation: TTL and renewal intervals must be safe integer
    milliseconds in 1..2,147,483,647, with renewal strictly below TTL. EN/ZH
    docs now record defaults, derived behavior, range, relation, and ownership
    semantics.

V2 validation:

  • pnpm install --frozen-lockfile (CI=true) — passed
  • pnpm test — 56 files, 581 tests passed
  • pnpm typecheck — passed
  • pnpm lint — passed (existing warnings/info only)
  • pnpm build — passed
  • npm pack --dry-run — passed
  • git diff --check — passed
  • gitleaks git 3940d8a..bd24f67 — no leaks

Packaging disposition:

  • normal scripts-enabled source Git installation remains wired through the
    existing prepare script
  • a current Node 25.6.1/npm 11.9.0 clean-cache rerun hit npm's known optional
    dependency bug (npm/cli#4828) before tsdown could load the rolldown native
    binding; the same control run fails identically at reviewed head 3940d8a, so
    this is not introduced by the V2 corrective commit
  • IGNORE_SCRIPTS_GIT_SHA_INSTALL: PENDING
  • fork-only reproducible-dist runtime branch/revision: PENDING, to be created
    only after focused independent review of the new source head
  • no dist/ files are added to this upstream PR

Summary

  • surface rejected message handler promises through the existing public channel.on('error') path
  • make the existing in-memory processing lock configurable and renewable for the full queued/batched handler lifetime
  • preserve the single SeenCache/dedup authority and the existing cleanup order

Root cause

SafetyPipeline logged message handler failures but did not forward them to the channel's public error observer. Its processing lock also used a fixed expiry, so a handler or queued batch that outlived the original TTL could lose its in-flight exclusion before settling.

Changes

  • add an optional SafetyPipelineOptions.onError seam and wire it to the existing LarkChannel.emitError
  • isolate failures thrown by an error observer so cleanup still completes
  • add SafetyConfig.processingLock.ttlMs and renewIntervalMs, with fail-loud validation
  • renew acquired leases until handler settlement, including queue and batch waiting
  • stop renewal before the best-effort SeenCache mark, then release the lock
  • dispose renewal timers without adding another cache, dedup layer, or emitter
  • build ignored dist/ artifacts during Git dependency installation via prepare, so a pinned commit is directly consumable

No package version change is included.

Validation

  • pnpm test — 56 files, 562 tests passed
  • pnpm typecheck — passed
  • pnpm lint — passed
  • pnpm build — passed
  • pnpm pack, followed by installation from the tarball in an empty HOME — passed
  • npm install https://github.com/mayf3/channel-sdk-node.git#3940d8a67709902584f040fea1b4c2aa0418f135 in an empty HOME — passed
  • package import smoke tests for both ESM and CommonJS — passed

The added regressions cover queue-enabled and queue-disabled error delivery, a throwing error observer, renewal across multiple original TTL periods, concurrent duplicate suppression, queued lease renewal, post-settlement SeenCache/release behavior, timer disposal, and invalid configuration.

External runtime packaging note: source head bd24f6742513769c80b5401b96ad464d74dd2027 was independently reviewed PASS; fork-only runtime revision ab028f9dbcc09effbdfa4c9885cdcc1f5ecc623f exists separately on mayf3/channel-sdk-node, while this upstream PR still contains no dist/ artifacts.

@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants