Skip to content

Apply() in internal/handlers/issue_applications.go doesn't follow the race-safe idempotency pattern its sibling EnqueueFullSync() uses, letting concurrent duplicate requests post two GitHub comments #394

Description

@Jagadeeshftw

Description

SyncHandler.EnqueueFullSync in internal/handlers/sync.go documents and implements a specific race-prevention strategy for idempotency keys: it inserts the idempotency key row before executing the side effect, using ON CONFLICT (user_id, idempotency_key) DO NOTHING, and only proceeds to the actual side effect (INSERT INTO sync_jobs) if RowsAffected() == 1 — i.e. this request "won" the insert race:

// sync.go — EnqueueFullSync
tag, err := h.db.Pool.Exec(c.Context(), `
INSERT INTO idempotency_keys (user_id, idempotency_key, response_status, response_body, created_at, expires_at)
VALUES ($1, $2, $3, $4, now(), $5)
ON CONFLICT (user_id, idempotency_key) DO NOTHING
`, userID, idempotencyKey, ...)
...
if tag.RowsAffected() == 0 {
    // Concurrency win: another request inserted this key first.
    return c.Status(fiber.StatusAccepted).JSON(successResponse)
}
// Execute the underlying sync jobs insert

IssueApplicationsHandler.Apply in internal/handlers/issue_applications.go implements the same Idempotency-Key contract (checks the cache first, then executes the side effect, then caches the response) but does not use this insert-first pattern. It performs the cache-miss check, then unconditionally executes the side effect (posting a GitHub comment via gh.CreateIssueComment), and only writes the idempotency_keys row at the very end, after the comment has already been posted:

// issue_applications.go — Apply()
if idempotencyKey != "" {
    // ... cache lookup, cache miss falls through ...
}
...
ghComment, err := gh.CreateIssueComment(c.Context(), linked.AccessToken, fullName, issueNumber, commentBody)
...
if idempotencyKey != "" {
    // cache write happens here, AFTER the GitHub comment was already posted
    _, insertErr := h.db.Pool.Exec(c.Context(), `
INSERT INTO idempotency_keys (user_id, idempotency_key, response_status, response_body, created_at, expires_at)
VALUES ($1, $2, $3, $4, now(), now() + interval '24 hours')
ON CONFLICT (user_id, idempotency_key) DO NOTHING
`, userID, idempotencyKey, fiber.StatusOK, string(responseBodyJSON))

Two concurrent requests with the same Idempotency-Key (e.g. a client's automatic retry firing before the first response returns, or a double-click before the button disables) both pass the cache-miss lookup before either one writes the cache row, so both proceed to call gh.CreateIssueComment and post two duplicate "Grainlify Application" comments on the same GitHub issue — exactly the failure mode idempotency keys exist to prevent, and exactly what EnqueueFullSync's comment block explicitly calls out as "Concurrency Protection."

Requirements

  • Apply() must reserve the idempotency key (insert-first with ON CONFLICT ... DO NOTHING, checking RowsAffected()) before calling gh.CreateIssueComment, not after.
  • If another concurrent request already holds the key, Apply() must not post a second GitHub comment; it should either return the fresh confirmation once the winning request finishes, or (if the fixed-key contention is exceptional and no cached body exists yet) safely surface a "duplicate in flight" response rather than double-posting.
  • Client-provided keys and any request without an Idempotency-Key header must both remain correct — requests without an idempotency key intentionally have no dedup guarantee and are out of scope.

Suggested execution

  1. In internal/handlers/issue_applications.go's Apply(), add a reservation INSERT ... ON CONFLICT (user_id, idempotency_key) DO NOTHING immediately after the cache-miss determination and before gh.CreateIssueComment, mirroring sync.go's pattern (using a placeholder response_status/response_body initially, or a short-lived "in-flight" marker row).
  2. If RowsAffected() == 0 (another request already holds the key), return a 202/cached response rather than proceeding to post another comment.
  3. After the GitHub comment succeeds, UPDATE the reserved row with the real response body/status instead of a second INSERT.
  4. Add a concurrency regression test (see internal/handlers/projects_led_concurrency_test.go for this repo's existing pattern of testing concurrent handler calls) that fires two concurrent Apply() calls with the same Idempotency-Key against a fake GitHub client and asserts CreateIssueComment is invoked exactly once.

Acceptance criteria

  • Two concurrent Apply() requests with the same Idempotency-Key result in exactly one GitHub comment being posted.
  • The losing concurrent request receives a 200/202 response consistent with the idempotent contract, not an error.
  • A regression test proves single-comment behavior under concurrency.

Security notes

Not an auth bypass, but a data-integrity/abuse gap: this is the exact double-submission problem Idempotency-Key support was added to solve elsewhere in this codebase (sync.go), and its absence here lets a retrying or misbehaving client spam duplicate application comments on maintainers' GitHub issues.

Guidelines

  • Minimum 95% test coverage
  • Timeframe: 96 hours

Metadata

Metadata

Assignees

No one assigned

    Labels

    GrantFox OSSGrantFox open-source programMaybe RewardedGrantFox: potentially rewarded contributionOfficial Campaign | FWC26GrantFox official campaign issuebackendBackend / API workbugSomething isn't workingconcurrency

    Type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions