Repository navigation
Shubham Jakhete - Phase 1: Finishing followup fixes for PR 4558+1953: Fix Dark Mode, Date Selection, Time Selection and Scheduling the post for Mastodon (backend) - #2373
Conversation
Scheduling Mastodon posts failed because the Mastodon routes were never registered, so every request returned 404. The scheduler that sends posts at their scheduled time was also never started, and the routes were open to anyone without logging in. - Register mastodonRouter in routes.js - Start the Mastodon schedule job in server.js - Remove the unauthenticated exception for /api/mastodon and require the sendEmails permission used by the Announcements page - Return 400 for empty content or a missing, invalid, or past time, and 404 when deleting a post that does not exist - Add tests for the routes and the scheduler
shubhamjakhete
added
the
High Priority - Please Review First
SonarCloud flagged the permission check's catch block for ignoring the error it catches. - Log the error before returning 500 - Add a test for a failing permission check
adit24dhaya
left a comment
adit24dhaya
left a comment
There was a problem hiding this comment.
I reviewed 9fb3ee1 and the paired frontend #5593 at 28960109. Registering the routes, removing the anonymous exception, adding the sendEmails guard and validating scheduled times are useful fixes; the existing route/scheduler tests pass.
Please make scheduled publication safe before enabling this job at startup. The real scheduler does a find of due records, publishes each record, then deletes it, without an atomic claim or a stable remote idempotency key. Holding the first publication promise open and invoking the registered minute callback again makes the same record reach axios.post twice. Separately, accepting the first post but failing deleteOne leaves the record due, and the next run publishes it again. The sequential-success control publishes once. The worker code is pre-existing, but this PR newly starts it on every server process, so these are production risks introduced by activating it.
Use an atomic persisted claim/lease that works across callbacks and server instances, plus durable post-success recovery so a cleanup failure cannot republish. A stable Mastodon Idempotency-Key can help, but the documented key retention is only one hour, so it is not a substitute for durable delivery state. Add overlap, two-worker and post-success persistence-failure regressions.
Validation: ten isolated assertions passed against the real startMastodonScheduleJob callback and processScheduledPosts, with Axios, cron registration and all schedule persistence mocked. Both failure cases produced two accepted publication calls for one due record; the normal sequential control produced one. Existing Jest tests: 13/13 across the two changed suites, including permission denial, permission-check failure, schedule validation and delete-not-found cases. Babel compiled 773 files; changed-file ESLint zero errors/18 warnings and git diff --check passed. Paired frontend: 11/11 tests, light/dark actual-component browser checks, build 9,176 modules and applicable lint passed. No live scheduler/server, database connection, media upload or real external publication was used. Live OAuth/account scopes and real authenticated middleware integration remain unverified; route tests use a stand-in authenticated requestor.
Commit audit: both commits are focused, imperative, under 72 characters, without trailing periods, and include blank-separated what/why bodies. A relevant issue/PR reference in the bodies would make the follow-up history easier to trace.
| require('./cronjobs/pullRequestReviewJobs')(); | ||
| require('./jobs/analyticsAggregation').scheduleDaily(); | ||
| require('./cronjobs/bidWinnerJobs')(); | ||
| require('./cronjobs/mastodonScheduleJob').startMastodonScheduleJob(); |
There was a problem hiding this comment.
Starting this worker makes its pre-existing find → publish → delete sequence live. Two overlapping callbacks both publish the same due record; a successful remote post followed by a failed delete is also republished on the next run. I reproduced both with the real registered callback and mocked persistence/HTTP (two accepted calls versus one in the sequential control). Please add an atomic cross-worker claim and durable/idempotent delivery recovery before enabling the scheduler, with regressions for overlap and post-success persistence failure.
RichaSapre
left a comment
RichaSapre
left a comment
There was a problem hiding this comment.
Tested locally together with frontend PR #5593. Verified Post Now, scheduled publishing, past-time validation, editing, deletion, permissions/Volunteer restrictions, and the scheduler starting successfully. The backend Mastodon tests also passed locally: 2 test suites, 13 tests total.
I agree with the existing concern around the scheduler’s find → publish → delete flow. Overlapping scheduler runs could publish the same due record more than once, and a successful Mastodon publish followed by a failed database delete could leave the record to be published again on the next run.
I’d like to see the concurrency/idempotency case addressed before approval. It would also be valuable to add regression coverage for:
- two overlapping scheduler executions processing the same scheduled post;
- Mastodon publishing successfully but deletion of the scheduled record failing.
Testing/setup note: the documented Mastodon scopes were not sufficient for History/Scheduled during my testing; adding the necessary read scope resolved the authorization error.
Attached screenshots from local testing showing all test cases passed.
| expect(MastodonSchedule.deleteOne).toHaveBeenCalledWith({ _id: 'p1' }); | ||
| }); | ||
|
|
||
| it('keeps a post in the schedule when posting fails', async () => { |
There was a problem hiding this comment.
Could we add regression tests for the other partial-failure/concurrency cases as well: overlapping scheduler executions and a successful Mastodon publish followed by a failed DB delete?
AaditTrivedi
left a comment
AaditTrivedi
left a comment
There was a problem hiding this comment.
Reviewed by Aadit Trivedi.
Backend pair of frontend #5593, which I reviewed and approved earlier.
How I verified
Checked out the PR (last commit Sep 30, 2026), confirmed no conflicts with development using git merge-tree, ran mastodonScheduleJob.test.js and mastodonRouter.test.js (13/13 passing), and reviewed the changes to the router, middleware, routes, and server startup.
Verified
src/startup/middleware.jsremoves the block that let any request starting with/api/mastodonskip authentication ("Allow unauthenticated access for Mastodon test APIs"), so the Mastodon endpoints now require login like the rest of the API. This closes an open, unauthenticated path to posting.- The Mastodon router is registered in
src/startup/routes.js. - The scheduled-post job is started from
src/server.js.
Pointer, not blocking
- Because the scheduler starts in
server.js, it runs in every backend instance. If the API is ever run with more than one instance, each would publish the same scheduled posts. A lock, or claiming each post atomically before publishing, would prevent duplicate posts.
Approving.
The scheduler found due posts, published each one and then deleted it, with no claim. This PR starts the scheduler on every server process, so an overlapping run or a second server could publish the same post again, and a failed delete after a successful post left it due for the next run. - Claim each due post atomically with findOneAndUpdate, moving it from pending to publishing with a lease, so only one worker sends it - Record a successful post as posted with its Mastodon status ID instead of deleting it - Mark a post failed, not resent, if its claim expires before delivery is recorded or the request times out - Retry clear Mastodon rejections (429, 5xx) on later runs, up to three attempts - Send a stable Idempotency-Key with each post - Hide posted records from the Scheduled list and only allow deleting pending or failed posts - Add tests for overlapping runs, two workers and a failed save after a successful post Related to OneCommunityGlobal/HighestGoodNetworkApp#5593
|
@adit24dhaya @RichaSapre Thanks both. Pushed 7061b5b: The scheduler now claims each due post atomically (pending → publishing with a lease), so overlapping runs or a second server can't pick up the same post. Success is recorded as posted instead of deleting the record. If saving that fails, the post is marked failed when its claim expires, not resent. Each post sends a stable Idempotency-Key. Only 429/5xx responses are retried, up to 3 attempts; timeouts are not retried. Added regressions for overlapping callbacks, two workers, and a successful post followed by a failed save. 20/20 tests pass across the two suites. Tested with mocked persistence and HTTP only, not against a live database or Mastodon. |
adit24dhaya
left a comment
adit24dhaya
left a comment
There was a problem hiding this comment.
Thanks, Shubham. I re-tested 7061b5b. The original overlapping-worker and failed-save cases now publish only once in my mocked checks. The new claim and saved delivery state address those reports.
Two failure cases still need changes; the inline comments give the steps and suggested fixes:
- A slow image upload can finish and publish after the claim expires and the delete endpoint has already reported success.
- A gateway error can leave an already-published post pending. Retrying after the remote duplicate-protection window expires can publish it again.
Test evidence: your two Jest suites pass, 20/20. I also ran 10 isolated scenarios against the actual scheduler, including separate worker modules, failed saves, timeout/403 handling, the three-attempt limit, legacy records and the 20-post batch limit. Those checks confirm the fixes above and reproduce the two remaining failures. All persistence and HTTP were mocked; no real posts or database writes were made. Babel compiled 773 files; ESLint found zero errors and 39 warnings; the whitespace check passed.
Commit audit: the new commit is focused, has a 60-character imperative subject without a final period, explains what/why in a blank-separated body, and references #5593. Optional documentation cleanup: the description still says successful records are deleted and lists 12 tests; the new behavior keeps posted records and there are 20 tests.
I haven't re-approved the unchanged frontend #5593; its separate edit-failure feedback remains open.
| status: 'failed', | ||
| lastError: 'Delivery was not confirmed before the claim expired; not retried', | ||
| }, | ||
| $unset: CLEAR_LOCK, |
There was a problem hiding this comment.
[P2] Stop an expired worker before it can publish
I held the image-upload response open, advanced the test clock 11 minutes, and ran another worker. It marked the record failed here. I then called the real delete controller: it returned 200, 'Scheduled post deleted successfully'. When I released the upload response, the original worker still sent the status to Mastodon, with no schedule record left.
The image-upload request has no timeout; only the later status request gets the 30-second timeout. Expiring the database claim doesn't stop that waiting worker. Please bound/cancel the upload and prevent a worker that has lost its claim from sending the status. Add a regression proving that this expired-and-deleted post is not published. This reproduction uses mocked HTTP and persistence, not a live account.
| // connection may mean the post went through, so it is not retried. | ||
| function isRetryable(err) { | ||
| const status = err.response?.status; | ||
| return status === 429 || status >= 500; |
There was a problem hiding this comment.
[P2] Don't treat every 5xx response as proof the post was rejected
A gateway can return 502/504 after the upstream server has accepted the post. In my mock, the first request creates the remote post but returns 502. This branch puts it back in pending. After a simulated 61-minute outage, the next run creates a second post with the same key. The one-minute retry control produces only one.
Mastodon documents key retention of up to one hour, so a three-attempt limit doesn't make a delayed retry safe. Please keep ambiguous gateway failures out of automatic retries, or add recovery that verifies delivery before resending. Add a regression for a retry after key expiry. The HTTP failure and remote cache are simulated; no live Mastodon request was made.
AaditTrivedi
left a comment
AaditTrivedi
left a comment
There was a problem hiding this comment.
Re-reviewed by Aadit Trivedi.
Follow-up to my earlier approval, checking the Oct 4 commit "Prevent scheduled posts from publishing twice," which addresses the multi-instance pointer from my review.
How I verified
Checked out the latest commit, ran mastodonScheduleJob.test.js and mastodonRouter.test.js (20/20 passing), and reviewed the scheduler, controller, and model changes.
Verified
- Scheduled posts now carry a status (pending, publishing, posted, failed), and the job claims each post atomically with
findOneAndUpdatebefore publishing, so with more than one backend instance only one can claim a given post. - Claims expire through
lockedUntil.failExpiredClaimsmarks a post failed if its delivery wasn't confirmed before the claim expired, and does not resend it, so a lost response can't produce a duplicate. Only a clear rejection from Mastodon is retried. That's a sound trade-off. - Posts that are publishing or already posted can't be deleted, and posted records are kept as history instead of being listed as scheduled.
- The tests use an in-memory stand-in that applies updates one at a time, which exercises the concurrent-claim case directly.
Pointer, not blocking
- Because an unconfirmed post is marked failed rather than retried, users need to see that to reschedule it. Worth making sure the frontend (#5593) shows failed posts and their
lastErrorin the History or Scheduled tab, so a post isn't silently dropped.
Approving.Re-reviewed by Aadit Trivedi.
Follow-up to my earlier approval, checking the Oct 4 commit "Prevent scheduled posts from publishing twice," which addresses the multi-instance pointer from my review.
How I verified
Checked out the latest commit, ran mastodonScheduleJob.test.js and mastodonRouter.test.js (20/20 passing), and reviewed the scheduler, controller, and model changes.
Verified
- Scheduled posts now carry a status (pending, publishing, posted, failed), and the job claims each post atomically with
findOneAndUpdatebefore publishing, so with more than one backend instance only one can claim a given post. - Claims expire through
lockedUntil.failExpiredClaimsmarks a post failed if its delivery wasn't confirmed before the claim expired, and does not resend it, so a lost response can't produce a duplicate. Only a clear rejection from Mastodon is retried. That's a sound trade-off. - Posts that are publishing or already posted can't be deleted, and posted records are kept as history instead of being listed as scheduled.
- The tests use an in-memory stand-in that applies updates one at a time, which exercises the concurrent-claim case directly.
Pointer, not blocking
- Because an unconfirmed post is marked failed rather than retried, users need to see that to reschedule it. Worth making sure the frontend (#5593) shows failed posts and their
lastErrorin the History or Scheduled tab, so a post isn't silently dropped.
Approving.
There was a problem hiding this comment.
Reviewed by Prajwal
Tested locally on Windows with frontend PR #5593. Requesting changes for one setup issue and a placeholder, everything else works well.
What I verified :
- Mastodon routes now require login. Unauthenticated GET /api/mastodon/schedule returns 401 (it returned 404 on development).
- Scheduler starts on server startup.
- Functional flows work: Post Now publishes to Mastodon; scheduled posts publish on time and are removed from the Scheduled tab; scheduling in the past returns "Scheduled time must be in the future"; delete works; Volunteer has no access.
- Tests pass: 2 suites, 20 tests.
Test Suites: 2 passed, 2 total
Tests: 20 passed, 20 total
Required changes :
- Setup instructions missing read scopes. :- Following the PR's instructions exactly (token with only write:statuses and write:media), the History tab fails: GET /api/mastodon/history?limit=20 returns {"error":"This action is outside the authorized scopes"}, shown as a red toast on the Mastodon page. The history endpoint reads posts from Mastodon, which requires read scopes. Please add the required scopes (likely read:statuses and read:accounts) to the How to test section, and to any production setup docs, since the organization's real token will need them too.
- Placeholder in How to test. :- Step 1 says HighestGoodNetworkApp#FRONTEND; please replace it with #5593.
Suggestion (non-blocking)
The description says 12 tests pass, but 20 run. Please update the number.
Two failure cases could still publish a scheduled post twice or after it was deleted (review on #2373): - A slow image upload could outlast the claim. Another run then marked the post failed, the user could delete it, and the original worker still published it once the upload finished. - A gateway or server error (5xx) can come back after Mastodon has accepted the post. It was put back in pending and retried, so a retry sent after the Idempotency-Key expired (Mastodon keeps it for up to one hour) could publish it a second time. Changes: - Renew the claim right before the status request and skip the post if the claim was lost, so an expired or deleted post is never sent - Pass the 30-second request timeout to the image upload and alt text requests, which had no timeout - Retry only 429 responses, where nothing was posted. Mark 5xx responses failed with lastError, like timeouts, so the user can reschedule instead of risking a duplicate - Add regression tests for the expired-and-deleted post, each 5xx status, a retry after the key window, and the 429 retry limit Refs #2373, OneCommunityGlobal/HighestGoodNetworkApp#5593
|
Pushed 86f04fc to address the review feedback:
27/27 tests pass in the two Mastodon suites. The new tests mock HTTP and the database. |
|
Follow-up to review on #5593 and the scheduler changes in OneCommunityGlobal/HGNRest#2373. - Editing a scheduled post saves the new version, then deletes the old one. If the delete failed, both stayed scheduled while the UI said "Failed to schedule post." Report that case on its own: the changes were saved and the older version must be deleted - Show an error on the Scheduled tab when the list fails to load, instead of "No scheduled posts yet" - The backend now marks a post failed when delivery can't be confirmed and does not retry it. Show failed posts with their error so the user can reschedule, and disable actions on a post that is being published - Remove the console.error calls flagged by the lint check - Add tests for the edit cases, the load error, and failed and publishing posts Refs #5593, OneCommunityGlobal/HGNRest#2373
vidiyala99
left a comment
vidiyala99
left a comment
There was a problem hiding this comment.
Re-checked the new head 86f04fc ("Stop expired and uncertain posts from republishing"). The PR's mocked tests can't show how the claim behaves on a real database, so I ran processScheduledPosts against an in-memory MongoDB with the real mastodonSchedule model, mocking only axios (the Mastodon API) and uploadMedia. Nothing was sent to Mastodon.
| Scenario | Mastodon calls | Final state |
|---|---|---|
| A. One due post, three workers run at the same time (each call takes 300 ms) | 1 | posted, remoteStatusId saved, attempts 1 |
| B. Mastodon answers 500, then another run | 1 | failed, lastError "mock 500" (not retried) |
| C. Mastodon answers 429 every time, five runs | 3 | failed after attempts 3 (MAX_ATTEMPTS) |
D. A publishing claim whose lease already expired |
0 | failed (never resent) |
E. A legacy record saved before status existed |
1 | posted, sent with Idempotency-Key: hgn-mastodon-schedule-<id> |
So the claim is atomic across workers, uncertain outcomes (5xx, expired lease) are never republished, only 429 is retried, and old records still go out. The PR's own suites also pass (mastodonScheduleJob.test.js + mastodonRouter.test.js, 2 suites, 27/27).
One suggestion, not blocking: a post that Mastodon accepted but whose result couldn't be saved ends up failed once its lease expires, which is the right call to avoid a double post. But a user who sees "failed" and reschedules would publish it twice. Showing lastError on the Scheduled tab with a hint like "This may already be live; check Mastodon before rescheduling" (when the lease expired) would cover that.
Approving.
adit24dhaya
left a comment
adit24dhaya
left a comment
There was a problem hiding this comment.
Thanks, Shubham. I rechecked 86f04fc5 against my earlier reproductions. Both remaining blockers are fixed: an upload that resumes after the claim expires and the post is deleted sends no status, and a gateway error no longer causes a second publication after a simulated 61-minute outage.
The overlap and failed-save controls still publish once. Rate-limit responses stop after three attempts, and legacy records still work.
Validation: 27/27 Jest tests passed. My independent checks ran 10 scenarios with 31 assertions against the actual scheduler and controller, with HTTP and persistence mocked. Babel compiled 773 files; ESLint reported 0 errors/41 warnings; the whitespace check passed. Frontend #5593 also passed its tests and browser checks. No real Mastodon posts or shared database writes were made; live token scopes and real database behavior were not tested here.
Approving. Small documentation note: this backend PR's current description appears to contain the frontend description, so please restore the backend-specific setup instructions.
Commit audit: the follow-up is focused, has a 65-character imperative subject without a final period, and includes a blank-separated explanation and relevant PR references.
There was a problem hiding this comment.
Reviewed on this PR's branch (pr-2373) at the latest commit 86f04fc5, backend only. Verified the three backend fixes with the tests, the server log, and an unauthenticated request. I didn't test posting to Mastodon, which needs a Mastodon access token, or the UI, which needs frontend #5593.
-
Tests: 27/27 passing
src/routes/__tests__/mastodonRouter.test.js: 10/10 (permission allowed/denied, valid and invalid scheduling including past and invalid times returning 400, delete found/not found)src/cronjobs/__tests__/mastodonScheduleJob.test.js: 17/17 (posts due items, skips posts not yet due, retry limits, no duplicate publish after a gateway error)
Minor: the description says 12 tests; the suites now contain 27, after the follow-up commits.
-
Scheduler starts
Onnpm start, the log showsMastodon schedule cron job started (runs every minute). -
Routes are registered and require login
curl.exe -i http://localhost:4500/api/mastodon/schedulewithout a token returnsHTTP/1.1 401 Unauthorizedwith{"error":"Unauthorized request: No header"}. The route now exists (previously 404) and no longer allows unauthenticated access.
Notes (non-blocking):
- Step 1 of the test steps still says frontend PR
#FRONTENDinstead of #5593. - SonarQube reports the Quality Gate failed on a B maintainability rating for new code.
Screenshots:
Router tests (10/10):
Scheduler tests (17/17):
Server log, cron job started:
Unauthenticated request returns 401:
Approving.
This branch has not been deployed
7 participants
Description
Bug list: "Phase 1: Finishing Followup fixes for the PR 4558+1953 Fix Dark Mode, Date Selection, Time Selection and Scheduling the post for Mastodon" (priority high, Yeshwanth).
Issues found when testing the Mastodon composer (Other Links → Send Emails → Mastodon):
Causes on the frontend:
fetch('/api/mastodon/...')on the frontend's own address, so requests never reached the backend (locally the Vite proxy also strips/api; on Netlify the request returnsindex.html).SocialMediaComposer.module.cssbut used plain class names, so none of its styles were applied: the sub-tabs ran together as "ComposerScheduledHistoryDetails", the inputs were unstyled, and the existing dark styles never applied. They also only followed the operating system's dark mode, not the app's.Related PRS (if any):
This frontend PR is related to backend PR #2373.
To test this frontend PR you need to check out backend PR #2373.
Builds on #4558 and #1953.
Main changes explained:
src/utils/URL.js: addMASTODON_POST,MASTODON_SCHEDULED_POSTS,MASTODON_SCHEDULED_POST_BY_ID, andMASTODON_POST_HISTORY, following the LinkedIn entries.SocialMediaComposer.jsx:fetch('/api/mastodon/...')calls withaxiosand the new endpoints, which sends the logged-in user's token.lastError). The backend (Shubham Jakhete - Phase 1: Finishing followup fixes for PR 4558+1953: Fix Dark Mode, Date Selection, Time Selection and Scheduling the post for Mastodon (backend) #2373) marks a post failed when delivery can't be confirmed and does not retry it, so the user can reschedule or delete it. Edit, Post now and Delete are disabled while a post is being published.styles['post-card'], etc.). Bootstrap button classes are unchanged.state.theme.darkMode) and add acomposer-darkclass.console.errorcalls.SocialMediaComposer.module.css:prefers-color-scheme: darkblock to.composer-darkrules so dark mode follows the app's setting.color-scheme: darkon the date and time inputs so the calendar and clock popups and icons are drawn in dark colors.scheduleTime.js: new helper that reads a scheduled post's date and time in local time for editing.How to test:
.envsetup).npm install, thennpm run start:local.npx vitest run src/components/Announcements/platforms/social.