Skip to content

Commit f3250ca

Browse files
committed
fix(worker): bound flakyActivity's effective retry policy in the fixture
The activityOptionsByName override in activity-options.workflows.ts shallow-replaces flakyActivity's contract-level retry block (by design, to prove shallow merge doesn't inherit maximumAttempts from the layer below) but supplied no maximumAttempts of its own, leaving the EFFECTIVE policy Temporal actually runs with unbounded. A regression that broke the "succeeds on retry" path would hang the test to its 120s timeout instead of failing an assertion. Add an explicit maximumAttempts: 2 to the override — exactly what the "fails once, succeeds on retry" implementation needs — without defeating the shallow-merge proof: the override's own bound (2) still visibly wins over the contract-level one (1), it's just no longer implicitly unbounded in between. Also soften activity-options.contract.ts's doc comment, which asserted from an informal, unreproduced probe that the time-skipping server skips retry backoff delay. That claim doesn't matter to correctness anymore: the explicit cap bounds retries regardless of backoff timing.
1 parent 2b01acc commit f3250ca

2 files changed

Lines changed: 30 additions & 16 deletions

File tree

packages/worker/src/__tests__/activity-options.contract.ts

Lines changed: 19 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -92,18 +92,25 @@ const usesDefaultActivity = defineActivity({
9292
* won), the override would never take effect.
9393
* - Overrides shallow-merge, they don't deep-merge into the layer below —
9494
* the contract-level `retry` here caps `maximumAttempts` at 1. The
95-
* workflow's override supplies a *different* `retry` field
96-
* (`backoffCoefficient`) and says nothing about `maximumAttempts`. If the
97-
* merge were deep (field-by-field), the lower layer's `maximumAttempts: 1`
98-
* would survive inside the merged `retry` and the activity would be
99-
* allowed exactly one attempt — permanently failing, since the
100-
* implementation only succeeds on the second call. Because the real merge
101-
* is shallow, the override's `retry` object replaces the contract-level
102-
* one *wholesale*: `maximumAttempts` is gone, Temporal's own (unbounded)
103-
* default applies, a second attempt happens, and the workflow completes.
104-
* A scratch probe against the real time-skipping server confirmed this
105-
* resolves in well under a second — server-side retry backoff is skipped
106-
* the same way workflow timers are.
95+
* workflow's override (`activity-options.workflows.ts`) supplies its own
96+
* `retry` block (`backoffCoefficient` + an explicit `maximumAttempts: 2`)
97+
* rather than inheriting anything from this layer. If the merge were deep
98+
* (field-by-field), the lower layer's `maximumAttempts: 1` would survive
99+
* inside the merged `retry` and the activity would be allowed exactly one
100+
* attempt — permanently failing, since the implementation only succeeds on
101+
* the second call. Because the real merge is shallow, the override's
102+
* `retry` object replaces the contract-level one *wholesale*: this
103+
* layer's `maximumAttempts: 1` is gone, the override's own `2` applies
104+
* instead, a second attempt happens, and the workflow completes.
105+
*
106+
* Whether the time-skipping server also skips retry BACKOFF DELAY the same
107+
* way it skips workflow timers is a claim this suite does not rely on —
108+
* an earlier version of this comment asserted it from an informal, un-
109+
* reproduced probe, which is not evidence this test's correctness should
110+
* depend on. The explicit `maximumAttempts: 2` on the override makes that
111+
* moot either way: retries are bounded regardless of backoff timing, so a
112+
* merge-precedence regression fails the assertion instead of hanging the
113+
* test to its timeout.
107114
*/
108115
const flakyActivity = defineActivity({
109116
input: z.object({}),

packages/worker/src/__tests__/activity-options.workflows.ts

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -43,10 +43,17 @@ export const resolvesLayeredOptions = declareWorkflow({
4343
activityOptions: { startToCloseTimeout: "30 seconds", retry: { maximumAttempts: 1 } },
4444
// `flakyActivity`'s contract-level `activityOptions` caps retries at 1
4545
// attempt. This override replaces that `retry` block wholesale (shallow
46-
// merge — it does not carry `maximumAttempts` forward), so retries are
47-
// effectively unbounded again; see the doc comment on `flakyActivity` in
48-
// the contract for the full argument.
49-
activityOptionsByName: { flakyActivity: { retry: { backoffCoefficient: 1 } } },
46+
// merge — it does not carry the lower layer's `maximumAttempts: 1`
47+
// forward); see the doc comment on `flakyActivity` in the contract for the
48+
// full argument. The override supplies its OWN `maximumAttempts: 2` rather
49+
// than leaving it unset: `flakyActivity`'s implementation fails once then
50+
// succeeds, so 2 attempts is exactly what the happy path needs, and capping
51+
// it (instead of falling through to Temporal's unbounded default) turns a
52+
// merge-precedence regression that broke the "succeeds on retry" path into
53+
// a fast assertion failure instead of a 120s test-timeout hang.
54+
activityOptionsByName: {
55+
flakyActivity: { retry: { backoffCoefficient: 1, maximumAttempts: 2 } },
56+
},
5057
implementation: async (context, args) => {
5158
const [contractTimeout, usesDefault, flaky, globalTimeout] = await Promise.all([
5259
context.activities.contractTimeoutActivity({ sleepMs: args.sleepMs }),

0 commit comments

Comments
 (0)