Skip to content

fix: resolve traceroute race condition for fast-responding nodes (#2364) #2366

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Yeraze merged 1 commit into from
Mar 22, 2026
Merged

fix: resolve traceroute race condition for fast-responding nodes (#2364) #2366

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 33 additions & 36 deletions src/services/database.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5314,48 +5314,45 @@ class DatabaseService {
}
}

recordTracerouteRequest(fromNodeNum: number, toNodeNum: number): void {
async recordTracerouteRequest(fromNodeNum: number, toNodeNum: number): Promise<void> {
Copy link
Copy Markdown

@github-actions github-actions bot Mar 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Great fix! Making this method properly async resolves the race condition. The previous fire-and-forget IIFE meant fast traceroute responses could arrive before the pending record was inserted into the database.

Consider adding a brief comment here explaining why the await is critical:

Suggested change
async recordTracerouteRequest(fromNodeNum: number, toNodeNum: number): Promise<void> {
async recordTracerouteRequest(fromNodeNum: number, toNodeNum: number): Promise<void> {
// NOTE: This MUST be awaited to prevent race conditions with fast-responding nodes
const now = Date.now();

const now = Date.now();

// For PostgreSQL/MySQL, use async repository
if (this.drizzleDbType === 'postgres' || this.drizzleDbType === 'mysql') {
Comment on lines 5320 to 5321
Copy link
Copy Markdown

@github-actions github-actions bot Mar 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Perfect removal of the problematic pattern. The fire-and-forget IIFE was the root cause of the race condition. Now the database operations are properly awaited before the method returns, ensuring the pending traceroute record exists when fast responses arrive.

// Fire async operations
(async () => {
try {
// Update the nodes table with last request time
if (this.nodesRepo) {
await this.nodesRepo.updateNodeLastTracerouteRequest(toNodeNum, now);
}
try {
// Update the nodes table with last request time
if (this.nodesRepo) {
await this.nodesRepo.updateNodeLastTracerouteRequest(toNodeNum, now);
}

// Insert a pending traceroute record
if (this.traceroutesRepo) {
const fromNodeId = `!${fromNodeNum.toString(16).padStart(8, '0')}`;
const toNodeId = `!${toNodeNum.toString(16).padStart(8, '0')}`;

await this.traceroutesRepo.insertTraceroute({
fromNodeNum,
toNodeNum,
fromNodeId,
toNodeId,
route: null, // null for pending (findPendingTraceroute checks for isNull)
routeBack: null,
snrTowards: null,
snrBack: null,
timestamp: now,
createdAt: now,
});
// Insert a pending traceroute record
if (this.traceroutesRepo) {
const fromNodeId = `!${fromNodeNum.toString(16).padStart(8, '0')}`;
const toNodeId = `!${toNodeNum.toString(16).padStart(8, '0')}`;

await this.traceroutesRepo.insertTraceroute({
fromNodeNum,
toNodeNum,
fromNodeId,
toNodeId,
route: null, // null for pending (findPendingTraceroute checks for isNull)
routeBack: null,
snrTowards: null,
snrBack: null,
timestamp: now,
createdAt: now,
});

// Cleanup old traceroutes
await this.traceroutesRepo.cleanupOldTraceroutesForPair(
fromNodeNum,
toNodeNum,
TRACEROUTE_HISTORY_LIMIT
);
}
} catch (error) {
logger.error('[DatabaseService] Failed to record traceroute request:', error);
// Cleanup old traceroutes
await this.traceroutesRepo.cleanupOldTraceroutesForPair(
fromNodeNum,
toNodeNum,
TRACEROUTE_HISTORY_LIMIT
);
}
})();
} catch (error) {
logger.error('[DatabaseService] Failed to record traceroute request:', error);
}
return;
}

Expand Down Expand Up @@ -10206,7 +10203,7 @@ class DatabaseService {

// Group 4: Traceroutes
async recordTracerouteRequestAsync(fromNodeNum: number, toNodeNum: number): Promise<void> {
this.recordTracerouteRequest(fromNodeNum, toNodeNum);
await this.recordTracerouteRequest(fromNodeNum, toNodeNum);
Copy link
Copy Markdown

@github-actions github-actions bot Mar 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Critical fix! This change from fire-and-forget to properly awaited call is essential. The async wrapper pattern throughout the codebase expects the inner method to be awaited, and this was the missing piece causing the race condition.

This now properly aligns with the multi-database architecture requirement that "ALL database methods in DatabaseService must be async".

}

async getAllTraceroutesForRecalculationAsync(): Promise<any[]> {
Expand Down
Loading