Skip to content

fix(propeller): clamp ArrayNode subNode delta timestamps instead of panicking - #27

Open
pfernandes21 wants to merge 1 commit into
masterfrom
devin/1789149969-arraynode-clamp-delta-timestamp
Open

pfernandes21 wants to merge 1 commit into
masterfrom
devin/1789149969-arraynode-clamp-delta-timestamp

Conversation

@pfernandes21

Copy link
Copy Markdown

Why are the changes needed?

FlytePropellerPanic on delphi-production (2026-08-29, recurred 2026-09-04) is a CompactArray invariant violation in the ArrayNode handler:

bitarray.(*CompactArray).validateValue   compact_array.go:34
bitarray.(*CompactArray).SetItem(..., 0x0, 0x400e6)   compact_array.go:41
array.(*arrayNodeHandler).Handle          handler.go:422

0x400e6 = 262 374 s. SubNodeDeltaTimestamps is sized from array-node.max-delta-timestamp (default 3d = 259 200 s), so a map-task subNode that only got scheduled ~3d 53m after its ArrayNode started (the flytesnacks-staging queue has been GPU/CPU-bound for days) produced an offset that does not fit the slot. SetItem panics, the round's recover() counts it as PanicObserved, and the workflow fails every subsequent round — the panic is deterministic for that state, so the execution never progresses.

What changes were proposed in this pull request?

  • bitarray.CompactArray.MaxValue() — exposes the per-slot ceiling validateValue already computes.
  • array.clampedDeltaSeconds(delta, maxDelta) — pure helper: negative → 0 (unset), otherwise min(seconds, maxDelta).
  • arrayNodeHandler.Handle stores the clamped value and logs a warning when clamping occurs. The only effect of clamping is that the subNode's reconstructed startedAt (used for event timestamps) saturates at max-delta-timestamp instead of failing the round.
-deltaDuration := uint64(subNodeStartedAt.Time.Sub(startedAt.Time).Seconds())
-arrayNodeState.SubNodeDeltaTimestamps.SetItem(index, deltaDuration)
+delta := subNodeStartedAt.Time.Sub(startedAt.Time)
+maxDelta := arrayNodeState.SubNodeDeltaTimestamps.MaxValue()
+deltaSeconds := clampedDeltaSeconds(delta, maxDelta)
+if delta > 0 && deltaSeconds < bitarray.Item(delta/time.Second) { logger.Warnf(...) }
+arrayNodeState.SubNodeDeltaTimestamps.SetItem(index, deltaSeconds)

The previous uint64(negativeDuration.Seconds()) also wrapped to ~2^64 and would have panicked the same way if an ArrayNode's LastAttemptStartedAt ever moved past a subNode's; the helper handles that case too.

How was this patch tested?

(cd flytestdlib   && go test -count=1 ./bitarray/)                      # ok
(cd flytepropeller && go test -count=1 ./pkg/controller/nodes/array/)  # ok
(cd flytepropeller && go build ./...)

Added TestClampedDeltaSeconds (negative, zero, sub-second, in-range, at-max, and the production value 262374 s) and TestCompactArray_MaxValue.

Labels

fixed

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Link to Devin session: https://app.devin.ai/sessions/246008e5dbb94b2fa6f80f272412b704
Open in Devin Desktop: https://app.devin.ai/desktop/session/246008e5dbb94b2fa6f80f272412b704?variant=devin
Requested by: @pfernandes21

…anicking

SubNodeDeltaTimestamps is a CompactArray sized from array-node.max-delta-timestamp (default 3d). A subNode that starts later than that (map task queued behind cluster capacity for days) produced a value too large for the slot and CompactArray.SetItem panicked, failing every round for that workflow (FlytePropellerPanic on delphi-production 2026-08-29 and 2026-09-04, value 262374s vs max 259200s).

Saturate the stored offset at the array's MaxValue (and map negative offsets to 0) so the round completes; the subNode's reconstructed start time loses precision and a warning is logged when clamping occurs.

Assisted-by: Devin:claude-opus-4.6
@devin-ai-integration

Copy link
Copy Markdown

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment, CI, and merge conflict monitoring

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.

1 participant