Fix/db provider max attempts bypass#27
Closed
elpete wants to merge 36 commits intodevelopmentfrom
Closed
Conversation
Lucee can return the `stackTrace` property on an exception as a Java array of StackTraceElement objects rather than a simple string. Guard against this by checking isSimpleValue() before comparing to "" and by serializing complex values to JSON before inserting. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
reservedDate compared against pool.getTimeout() always used the pool's fixed 60s window, causing jobs with longer per-job timeouts (e.g. 300s) to be re-grabbed and have attempts incremented while still running. availableDate is already set to now + jobTimeout at reservation time, so comparing it against now correctly respects each job's actual timeout. Fixes both fetchPotentiallyOpenRecords and tryToLockRecords. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…timeout Three specs covering the core bug fix in DBProvider: - A reserved job within its own timeout is not re-grabbed - A reserved job whose timeout has expired is re-grabbed - A job past the pool timeout but within the job timeout is not re-grabbed Uses TestBox makePublic() to expose fetchPotentiallyOpenRecords for direct testing. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ockRecords guard ColdBoxAsyncProvider: The thenCompose closure referenced arguments.currentAttempt which does not exist in the closure scope, so setCurrentAttempt() never executed for retried jobs. Changed to check the captured attempts variable instead. DBProvider.tryToLockRecords: Added whereNotNull(reservedDate) guard to the availableDate OR branch, consistent with the same fix already applied to fetchPotentiallyOpenRecords. Ensures only genuinely timed-out reserved jobs are re-locked. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
If dispatchThenJobIfNeeded or dispatchCatchJobIfNeeded threw an exception (e.g. missing WireBox mapping, connection error), the finally job would never be dispatched. Wrap both in try/catch so dispatchFinallyJobIfNeeded always runs, matching the semantic contract of "finally". Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The exception was passed to the job onFailure handler under the misspelled key "excpetion" instead of "exception", so any job defining onFailure( exception ) would receive an undefined argument. Aligns SyncProvider with the AbstractQueueProvider behaviour. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
When the marshalJob future's exception handler fails to execute properly, jobs remain reserved with reservedDate set. Once the job timeout passes, fetchPotentiallyOpenRecords picks the row up again with no maxAttempts check, causing unbounded retries (observed: 29 attempts on a job configured for 3). Add a defensive guard at pickup time: before incrementing attempts and dispatching, compare the DB attempts column against maxAttempts. If exceeded, mark the job as failed and skip. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The marshalJob future was fire-and-forget. Any exception inside .onException — releaseJob serialization, batch recording, an interceptor, the job's onFailure hook — was silently absorbed by the unobserved future, leaving the row reserved and triggering unbounded timeout-based re-pickups. Wrap each side-effect in its own try/catch so one failure cannot prevent the rest from running, and add an outer catch that calls a new forceFailJob hook as a last-resort terminal kill. forceFailJob is a no-op on AbstractQueueProvider and overridden on DBProvider with a minimal UPDATE that sets failedDate and clears the reservation. It deliberately skips bookkeeping (onFailure hook, interceptors, batch recording) — it only runs when the proper failure path itself throws, and the alternative is the row being retried until manually cleaned up. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…empts integration tests The inline loop body is pulled into a private processLockedRecord method so integration tests can make it public and exercise the maxAttempts pre-flight guard in isolation. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
If log.error threw in the async thread (e.g. when serialising a MockBox exception struct), markJobFailed was silently skipped, leaving the row reserved and the timeout watcher free to re-pick it up indefinitely. Wrapping the log call in its own try/catch ensures the fallback path to markJobFailed is always reached regardless of logging failures. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
WireBox provider methods (newQuery) are not reliably invocable through MockBox pass-through in async threads, so the DB write in markJobAsFailedById silently failed in CI regardless of the log-guarding fix. Switching to a mock-call assertion — the same technique used by the forceFailJob-fallback test — verifies the correct code path without depending on the broken DB write path. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
MockBox cannot reliably stub private methods reached through the async closure's captured variables scope, and its pass-through for WireBox provider methods (newQuery) is broken in async threads. Replace the prepareMock approach with a real FailingReleaseDBProvider subclass that overrides releaseJob to throw — no mocking needed, so newQuery stays a proper WireBox provider method and the DB write goes through correctly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds tests/resources/app/models/**/*.cfc to the format, format:check, and format:watch scripts so fixture files like FailingReleaseDBProvider are checked and formatted consistently with the rest of the codebase. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Wrap afterJobHook in try/catch so a throwing hook logs via logSideEffectFailure and does not flip a successful job to failed - Rename onFailureExceptionIsExpcetion → onFailureExceptionHasExcpetionKey in OnFailureCapturingJob and SyncProviderOnFailureSpec to separate the intentional 'excpetion' typo-under-test from the variable naming Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…rceptor memento, properties, exceptionExtendedInfo, exceptionStackTrace, and exception all map to LONGTEXT in the cbq_failed_jobs schema. Binding them as CF_SQL_VARCHAR risks truncation or driver errors on large payloads (e.g. deep stack traces or complex job mementos). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.