Skip to content

fix: Stop silently discarding temporal activity rate limit configuration - #209

Open
laouji wants to merge 1 commit into
mainfrom
flows/activity-rate-limit
Open

laouji wants to merge 1 commit into
mainfrom
flows/activity-rate-limit

Conversation

@laouji

@laouji laouji commented Sep 8, 2026

Copy link
Copy Markdown
Contributor
Bug: the flows activity rate limit is a no-op

    cmd/worker.go:38 reads the flag with cmd.Flags().GetInt(...), but go-libs
    registers it as a Float64 (go-libs/v3@v3.3.0/temporal/cli.go:26). pflag's
    getFlagType rejects the type mismatch and returns 0, err
    (pflag/flag.go:411); flows discards the error, so
    TaskQueueActivitiesPerSecond: float64(0) → the SDK substitutes
    defaultTaskQueueActivitiesPerSecond = 100000. The operator's
    TEMPORAL_MAX_PARALLEL_ACTIVITIES=10
    (operator/internal/resources/orchestrations/deployments.go:173) has no
    effect. Worth noting for your question: if this were working, flows would
  be
    the bottleneck at 10 activities/s, not payments.

@laouji
laouji marked this pull request as ready for review September 8, 2026 15:56
@NumaryBot

NumaryBot commented Sep 8, 2026 •

Copy link
Copy Markdown
Contributor

✅ Approve — automated review

The patch uses the correct flag accessor, preserves the floating-point rate limit, and propagates errors through both worker startup paths. No actionable defects or corresponding repository documentation requiring updates were found. Tests could not be run because Go is unavailable in the environment.

No findings.

@shipfox-ai

shipfox-ai Bot commented Sep 8, 2026

Copy link
Copy Markdown

This PR fixes a real bug in cmd/worker.go: the --max-parallel-activities flag is registered as a float64 by go-libs, but was read with GetInt, whose error was silently discarded — so TaskQueueActivitiesPerSecond was always 0 and the rate limit never took effect. The change switches to GetFloat64, propagates the accessor error with a wrapped message, updates both worker startup paths (cmd/serve.go:72 for serve --worker and cmd/worker.go:69 for the worker command), and adds a regression test. I verified all call sites were updated and that the value now flows untransformed into worker.Options. No spec is linked; the PR's stated intent is fully implemented. Recommendation: approve, with one minor, non-blocking test comment below. (Cosmetic notes raised during review — e.g. the local workerOptions shadowing the package function at cmd/serve.go:72 / cmd/worker.go:69, and the repeated two-line error-check idiom — were verified and judged style-only with no correctness or compatibility impact, so they are not retained as findings.)

Standards

  1. Minor — regression-test does not assert the propagated value (cmd/worker_test.go:25-26). The new test asserts that cmd.Flags().GetFloat64 returns 10 and that workerOptions(cmd) returns no error, but never checks that the resulting worker.Options.TaskQueueActivitiesPerSecond carries that value. Mitigating factor: the test does catch the original bug, since reverting to GetInt on a float64 flag would make GetFloat64 fail and workerOptions return an error. However, a future regression that reads the flag correctly but drops the value again (e.g. hardcoding 0 in worker.Options) would still pass. Suggest asserting the propagated value (e.g. by introspecting the built fx.Option's worker configuration) or noting explicitly that this is a smoke test for the accessor type only.

No other confirmed material Standards finding. Both reviewers' remaining candidates (duplicated error-check idiom, variable/function shadowing, mixed strict/lenient flag reads, trailing nil return shape) were verified against the code and rejected as style-only with no correctness, security, compatibility, or risky-test impact. The pre-existing _-discard pattern on other flag reads is outside this diff and correctly left untouched.

Spec

No confirmed Spec finding — PASS. There is no linked issue; against the PR's own stated intent, all three implied requirements are implemented and verified in the code: (1) the flag is read with the correct float64 accessor (cmd/worker.go:39), (2) the accessor error is checked and propagated (cmd/worker.go:40-42), and (3) the value reaches worker.Options.TaskQueueActivitiesPerSecond untransformed (cmd/worker.go:47-49) via both startup paths. Scope is tight — no creep beyond the fix, its call sites, the fmt import, and the supporting test.

Reviewed independently by GLM (glm-5.3-flash) and DeepSeek (deepseek-v4-pro-0813) via Shipfox; verified and synthesized by GLM.

@laouji
laouji force-pushed the flows/activity-rate-limit branch from 972d7f6 to a0f246a Compare September 15, 2026 09:58

This branch has not been deployed

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants