Skip to content

Commit 15e8a19

Browse files
authored
fix: enforce turnstyle wait deadlines (#158)
* fix: enforce turnstyle wait deadlines Signed-off-by: Rui Chen <rui@chenrui.dev> * fix: harden deadline boundaries Signed-off-by: Rui Chen <rui@chenrui.dev> * test: cover GitHub API base URLs Signed-off-by: Rui Chen <rui@chenrui.dev> * fix: cancel deadline retry backoffs Signed-off-by: Rui Chen <rui@chenrui.dev> * fix: preserve retry policy boundaries Signed-off-by: Rui Chen <rui@chenrui.dev> * fix: preserve secondary rate-limit behavior Signed-off-by: Rui Chen <rui@chenrui.dev> * fix: preserve the initial discovery window Signed-off-by: Rui Chen <rui@chenrui.dev> * fix: propagate pagination abort signals Signed-off-by: Rui Chen <rui@chenrui.dev> * fix: enforce monotonic request checkpoints Signed-off-by: Rui Chen <rui@chenrui.dev> --------- Signed-off-by: Rui Chen <rui@chenrui.dev>
1 parent afaccda commit 15e8a19

20 files changed

Lines changed: 2793 additions & 306 deletions

AGENTS.md

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,17 @@
1818
- Do not reintroduce `@vercel/ncc` for routine builds; the current dependency set relies on package exports that ncc has previously failed to bundle correctly.
1919
- Keep the esbuild target aligned with the action runtime in `action.yml`.
2020

21+
## Wait Deadlines
22+
23+
- Create the shared `continue-after-seconds` or `abort-after-seconds` deadline in `main.ts` after parsing inputs and before the first Actions API read.
24+
- Treat the limit as a monotonic total-elapsed-time deadline across repository workflow lookup, workflow-run discovery, job and step reads, and sleeps.
25+
- Start the separate `initial-wait-seconds` window when workflow-run discovery begins; do not let repository workflow lookup consume it, and keep its sleep bounded by the shared action deadline.
26+
- Propagate the shared deadline signal and monotonic checkpoint through every potentially blocking Actions API read, including each pagination page and retry attempt.
27+
- Keep GitHub API retry backoffs on the shared abortable, chunk-safe scheduler so a deadline does not leave an Octokit retry timer running.
28+
- On an ordinary terminal failure, cancel the shared signal before reporting the original error; keep cancellation separate from deadline expiration and timer disposal.
29+
- Schedule long deadlines and sleeps in bounded timer chunks; never pass a delay above Node's timer maximum directly to `setTimeout`.
30+
- Use fake timers for deadline regression tests; do not add real sleeps.
31+
2132
## Docs
2233

2334
- Keep README inputs and outputs aligned with `action.yml` and the source input parsing.

CHANGELOG.md

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,13 @@
11
## Unreleased
22

3+
### Bug fixes 🐛
4+
5+
- Enforce `continue-after-seconds` and `abort-after-seconds` as total elapsed-time deadlines starting before the first Actions API read and covering workflow discovery, job and step reads, initial waiting, and polling.
6+
- Keep the `initial-wait-seconds` discovery window anchored to workflow-run discovery so repository workflow lookup does not consume its retry opportunity, while still bounding it by the action deadline.
7+
- Safely chunk long timer delays and check the monotonic boundary before follow-up discovery reads, pagination pages, and retries so timer overflow or delayed callbacks cannot shorten or extend the configured deadline.
8+
- Cancel queued GitHub API retry backoffs when the shared deadline expires or an ordinary terminal API failure ends the action, instead of allowing their timers to keep the step running.
9+
- Keep configured 5xx retries independent from the one primary-rate-limit retry, and avoid retrying primary limits with missing or malformed reset headers.
10+
311
## 3.3.2
412

513
### Bug fixes 🐛

README.md

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,12 @@ jobs:
5151

5252
To avoid waiting prolonged periods of time, you may wish to bail on a run or continuing a workflow run regardless of the status of the previous run.
5353

54+
The `continue-after-seconds` and `abort-after-seconds` limits measure total
55+
elapsed waiting time. The deadline starts after inputs are parsed and before
56+
the first Actions API read, then covers workflow and run discovery, job and
57+
step inspection, configured API retry backoffs, the initial wait, and polling
58+
sleeps. A deadline cancels a queued retry wait through the same request signal.
59+
5460
You can bail from a run using the built-in GitHub Actions [`jobs.<job_id>.timeout-minutes`](https://help.github.com/en/actions/automating-your-workflow-with-github-actions/workflow-syntax-for-github-actions#jobsjob_idtimeout-minutes) setting
5561

5662
```diff
@@ -259,12 +265,12 @@ jobs:
259265
| Name | Type | Description |
260266
| ------------------------ | ------- | ------------------------------------------------------------------------------------------------------------------------------------------- |
261267
| `token` | string | GitHub access token used for Actions API reads (defaults to `github.token`) |
262-
| `continue-after-seconds` | number | Maximum number of seconds to wait before moving forward (unbound by default). Mutually exclusive with abort-after-seconds |
263-
| `abort-after-seconds` | number | Maximum number of seconds to wait before aborting the job (unbound by default). Mutually exclusive with continue-after-seconds |
268+
| `continue-after-seconds` | number | Maximum elapsed seconds from before the first Actions API read, including later API reads and sleeps, before moving forward (unbound by default). Mutually exclusive with abort-after-seconds |
269+
| `abort-after-seconds` | number | Maximum elapsed seconds from before the first Actions API read, including later API reads and sleeps, before aborting the job (unbound by default). Mutually exclusive with continue-after-seconds |
264270
| `poll-interval-seconds` | number | Number of seconds to wait in between checks for previous run completion (defaults to 60) |
265271
| `same-branch-only` | boolean | Only wait on other runs from the same branch (defaults to true) |
266272
| `branch` | string | Branch name to use for same-branch filtering (defaults to the current branch) |
267-
| `initial-wait-seconds` | number | Total elapsed seconds within which period the action will refresh the list of current runs, if no runs were found in the first attempt |
273+
| `initial-wait-seconds` | number | Seconds from the start of workflow-run discovery within which the action refreshes current runs when none are found initially; bounded by any continue or abort deadline |
268274
| `job-to-wait-for` | string | Name of the workflow's job to wait for (unbound by default). |
269275
| `step-to-wait-for` | string | Name of the step to wait for (unbound by default). Requires job-to-wait-for to be set. |
270276
| `queue-name` | string | Custom substring used to group matching runs across workflows (defaults to the current workflow only). |

__tests__/deadline.test.ts

Lines changed: 218 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,218 @@
1+
import { afterEach, describe, expect, it, vi } from 'vitest';
2+
3+
import {
4+
ActionDeadline,
5+
DeadlineReached,
6+
MAX_TIMER_DELAY_MILLISECONDS,
7+
type DeadlineTiming,
8+
} from '../src/deadline';
9+
10+
interface ScheduledTimer {
11+
callback: () => void;
12+
milliseconds: number;
13+
}
14+
15+
const manualTiming = () => {
16+
let now = 0;
17+
let nextTimer = 1;
18+
const timers = new Map<number, ScheduledTimer>();
19+
const scheduledDelays: number[] = [];
20+
const clearedTimers: number[] = [];
21+
const timing: DeadlineTiming = {
22+
now: () => now,
23+
setTimeout: (callback, milliseconds) => {
24+
const timer = nextTimer++;
25+
timers.set(timer, { callback, milliseconds });
26+
scheduledDelays.push(milliseconds);
27+
return timer as unknown as ReturnType<typeof setTimeout>;
28+
},
29+
clearTimeout: (timer) => {
30+
const timerId = timer as unknown as number;
31+
timers.delete(timerId);
32+
clearedTimers.push(timerId);
33+
},
34+
};
35+
36+
return {
37+
timing,
38+
advanceTo: (milliseconds: number) => {
39+
now = milliseconds;
40+
},
41+
deliverNextTimer: () => {
42+
const entry = timers.entries().next().value as [number, ScheduledTimer] | undefined;
43+
if (!entry) {
44+
throw new Error('No timer is scheduled');
45+
}
46+
const [timer, scheduled] = entry;
47+
timers.delete(timer);
48+
scheduled.callback();
49+
return scheduled.milliseconds;
50+
},
51+
scheduledDelays,
52+
clearedTimers,
53+
timerCount: () => timers.size,
54+
};
55+
};
56+
57+
const deferred = <T>() => {
58+
let resolve!: (value: T) => void;
59+
let reject!: (error: unknown) => void;
60+
const promise = new Promise<T>((resolvePromise, rejectPromise) => {
61+
resolve = resolvePromise;
62+
reject = rejectPromise;
63+
});
64+
return { promise, reject, resolve };
65+
};
66+
67+
describe('ActionDeadline', () => {
68+
afterEach(() => {
69+
vi.restoreAllMocks();
70+
});
71+
72+
it.each(['continue', 'abort'] as const)(
73+
'chunks a long %s deadline without expiring before monotonic time reaches it',
74+
async (mode) => {
75+
const fake = manualTiming();
76+
const seconds = 2_147_484;
77+
const deadline = new ActionDeadline({ mode, seconds }, fake.timing);
78+
79+
expect(deadline.signal?.aborted).toBe(false);
80+
expect(fake.scheduledDelays).toEqual([MAX_TIMER_DELAY_MILLISECONDS]);
81+
82+
fake.advanceTo(MAX_TIMER_DELAY_MILLISECONDS);
83+
expect(fake.deliverNextTimer()).toBe(MAX_TIMER_DELAY_MILLISECONDS);
84+
85+
expect(deadline.signal?.aborted).toBe(false);
86+
expect(fake.scheduledDelays).toEqual([MAX_TIMER_DELAY_MILLISECONDS, 353]);
87+
88+
fake.advanceTo(seconds * 1000);
89+
fake.deliverNextTimer();
90+
91+
expect(deadline.signal?.aborted).toBe(true);
92+
await expect(deadline.race(async () => 'too late')).rejects.toEqual(
93+
new DeadlineReached(mode, seconds),
94+
);
95+
expect(fake.scheduledDelays.every((delay) => delay <= MAX_TIMER_DELAY_MILLISECONDS)).toBe(
96+
true,
97+
);
98+
},
99+
);
100+
101+
it('clears the currently armed timer during cleanup after a chunk re-arms', () => {
102+
const fake = manualTiming();
103+
const deadline = new ActionDeadline({ mode: 'abort', seconds: 2_147_484 }, fake.timing);
104+
105+
fake.advanceTo(MAX_TIMER_DELAY_MILLISECONDS);
106+
fake.deliverNextTimer();
107+
expect(fake.timerCount()).toBe(1);
108+
109+
deadline.dispose();
110+
111+
expect(fake.timerCount()).toBe(0);
112+
expect(fake.clearedTimers).toHaveLength(1);
113+
});
114+
115+
it('chunks a long polling sleep without scheduling a timer beyond the Node limit', async () => {
116+
const fake = manualTiming();
117+
const seconds = 2_147_484;
118+
const deadline = new ActionDeadline(undefined, fake.timing);
119+
const sleep = deadline.sleepSeconds(seconds);
120+
121+
expect(fake.scheduledDelays).toEqual([MAX_TIMER_DELAY_MILLISECONDS]);
122+
123+
fake.advanceTo(MAX_TIMER_DELAY_MILLISECONDS);
124+
fake.deliverNextTimer();
125+
for (let microtask = 0; microtask < 8; microtask += 1) {
126+
await Promise.resolve();
127+
}
128+
129+
expect(fake.scheduledDelays).toEqual([MAX_TIMER_DELAY_MILLISECONDS, 353]);
130+
131+
fake.advanceTo(seconds * 1000);
132+
fake.deliverNextTimer();
133+
134+
await expect(sleep).resolves.toBeUndefined();
135+
expect(fake.scheduledDelays.every((delay) => delay <= MAX_TIMER_DELAY_MILLISECONDS)).toBe(true);
136+
});
137+
138+
it('re-checks monotonic time when an operation resolves before the timer callback is delivered', async () => {
139+
const fake = manualTiming();
140+
const operation = deferred<string>();
141+
const deadline = new ActionDeadline({ mode: 'abort', seconds: 1 }, fake.timing);
142+
const result = deadline.race(() => operation.promise);
143+
144+
fake.advanceTo(1_001);
145+
operation.resolve('late value');
146+
147+
await expect(result).rejects.toEqual(new DeadlineReached('abort', 1));
148+
expect(deadline.signal?.aborted).toBe(true);
149+
});
150+
151+
it('re-checks monotonic time when an operation rejects before the timer callback is delivered', async () => {
152+
const fake = manualTiming();
153+
const operation = deferred<string>();
154+
const deadline = new ActionDeadline({ mode: 'continue', seconds: 1 }, fake.timing);
155+
const result = deadline.race(() => operation.promise);
156+
157+
fake.advanceTo(1_001);
158+
operation.reject(new Error('late API failure'));
159+
160+
await expect(result).rejects.toEqual(new DeadlineReached('continue', 1));
161+
expect(deadline.signal?.aborted).toBe(true);
162+
});
163+
164+
it('allows an operation that resolves immediately before the deadline', async () => {
165+
const fake = manualTiming();
166+
const operation = deferred<string>();
167+
const deadline = new ActionDeadline({ mode: 'continue', seconds: 1 }, fake.timing);
168+
const result = deadline.race(() => operation.promise);
169+
170+
fake.advanceTo(999);
171+
operation.resolve('on time');
172+
173+
await expect(result).resolves.toBe('on time');
174+
expect(deadline.signal?.aborted).toBe(false);
175+
});
176+
177+
it('treats the exact monotonic boundary as expired', async () => {
178+
const fake = manualTiming();
179+
const deadline = new ActionDeadline({ mode: 'continue', seconds: 1 }, fake.timing);
180+
181+
fake.advanceTo(1_000);
182+
183+
await expect(deadline.race(async () => 'boundary value')).rejects.toEqual(
184+
new DeadlineReached('continue', 1),
185+
);
186+
expect(deadline.signal?.aborted).toBe(true);
187+
});
188+
189+
it('cancels pending work idempotently without marking the configured deadline as reached', () => {
190+
const fake = manualTiming();
191+
const deadline = new ActionDeadline({ mode: 'abort', seconds: 10 }, fake.timing);
192+
const reason = new Error('terminal API failure');
193+
194+
deadline.cancel(reason);
195+
deadline.cancel(new Error('later cancellation'));
196+
197+
expect(deadline.signal?.aborted).toBe(true);
198+
expect(deadline.signal?.reason).toBe(reason);
199+
expect(() => deadline.throwIfReached()).not.toThrow();
200+
expect(fake.timerCount()).toBe(1);
201+
202+
deadline.dispose();
203+
expect(fake.timerCount()).toBe(0);
204+
});
205+
206+
it('preserves the configured deadline outcome when cancellation follows expiration', async () => {
207+
const fake = manualTiming();
208+
const deadline = new ActionDeadline({ mode: 'continue', seconds: 1 }, fake.timing);
209+
210+
fake.advanceTo(1_000);
211+
fake.deliverNextTimer();
212+
deadline.cancel(new Error('terminal API failure'));
213+
214+
await expect(deadline.race(async () => 'too late')).rejects.toEqual(
215+
new DeadlineReached('continue', 1),
216+
);
217+
});
218+
});

0 commit comments

Comments
 (0)