fix(replication): correct Postgres epoch constant in replication client - #4433
Conversation
|
|
Hi @deepshekhardas, thanks for your interest in contributing! This project requires that pull request authors are vouched, and you are not in the list of vouched users. This PR will be closed automatically. See https://github.com/triggerdotdev/trigger.dev/blob/main/CONTRIBUTING.md for more details. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| let [upperWAL, lowerWAL]: [number, number] = [parseInt(slice[0], 16), parseInt(slice[1], 16)]; | ||
| // Timestamp as microseconds since midnight 2000-01-01 | ||
| const now = Date.now() - 946080000000; | ||
| const now = Date.now() - POSTGRES_EPOCH_MS; |
There was a problem hiding this comment.
🔍 Standby status low-order timestamp word is still scaled in milliseconds, not microseconds
The acknowledge path builds a Postgres standby status update whose timestamp field must be microseconds since 2000-01-01, split into two 32-bit words. upperTimestamp correctly computes micros >> 32 (now / 4294967.296), but lowerTimestamp is computed as now - upperTimestamp * 4294967.296, which is still in milliseconds — it should be multiplied by 1000 to yield the low 32 bits of the microsecond value (internal-packages/replication/src/client.ts:846). The result is a reply timestamp that can be off by up to ~71 minutes in pg_stat_replication.reply_time. This is pre-existing and unrelated to the epoch constant fix, but sits directly adjacent to the changed line; worth fixing in the same pass since the PR is about timestamp correctness.
Was this helpful? React with 👍 or 👎 to provide feedback.
| /** | ||
| * Milliseconds between the Unix epoch (1970-01-01) and the Postgres epoch | ||
| * (2000-01-01), used to convert between Postgres replication timestamps and | ||
| * `Date.now()`. Mirrors `(POSTGRES_EPOCH_JDATE - UNIX_EPOCH_JDATE) * USECS_PER_DAY` | ||
| * in pgoutput.ts (946684800000000 micros). | ||
| */ | ||
| const POSTGRES_EPOCH_MS = 946684800000; |
There was a problem hiding this comment.
🔍 No .server-changes note for a server-affecting fix
internal-packages/replication ships only inside the webapp/server image (consumed by apps/webapp/app/services/runsReplicationService.server.ts and sessionsReplicationService.server.ts), so per AGENTS.md / CONTRIBUTING.md a .server-changes/ entry may be expected for server-only changes. The rule text enumerates apps/webapp/, apps/supervisor/, "etc.", so whether internal packages qualify is ambiguous; a maintainer should confirm. The behavioral impact is limited to the heartbeat timestamp value emitted to listeners, and neither consumer reads it (both destructure only lsn and shouldRespond), so the fix has no user-visible effect today.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughAdded the shared ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
1 participant
Rebased version of #4020. Corrects the Postgres epoch constant from \946080000000\ to \946684800000\ (2000-01-01) in the replication client, mirroring \POSTGRES_EPOCH_JDATE\ in pgoutput.ts. Adds a named \POSTGRES_EPOCH_MS\ constant with docs. Closes #4020.