RPC client: resend to a rediscovered server only on 401/403/404/410; a 504 apply may still be running

Author: ualtinok

docs/sources.md

@@ -1281,7 +1281,11 @@ Pending provenance (excluded from the passing-test table): `persists an approved
 | rpc | Authentication rejection rediscovers a replacement in the same call | new (neither copy) | stale status 401 rediscovery succeeds in the same pending call |
 | rpc | Forbidden response rediscovers a replacement in the same call | new (neither copy) | stale status 403 rediscovery succeeds in the same pending call |
 | rpc | Stale route response rediscovers a replacement in the same call | new (neither copy) | stale status 404 rediscovery succeeds in the same pending call |
-| rpc | Unavailable response rediscovers a replacement in the same call | new (neither copy) | stale status 503 rediscovery succeeds in the same pending call |
+| rpc | Gone response rediscovers a replacement in the same call | new (neither copy) | stale status 410 rediscovery succeeds in the same pending call |
+| rpc | Internal error is not resent to another server | new (neither copy) | status 500 is not resent to another server |
+| rpc | Bad gateway is not resent to another server | new (neither copy) | status 502 is not resent to another server |
+| rpc | Unavailable is not resent to another server | new (neither copy) | status 503 is not resent to another server |
+| rpc | An apply past its deadline is not resent to another server | new (neither copy) | status 504 is not resent to another server |
 | rpc | Cached non-strict selection cannot route an exact dead PID to a fallback server | new (neither copy) | cached selections never weaken exactPid refusal for a dead PID |
 | fs | Test cleanup skips completed releases without changing deliberate repeated calls | new (neither copy) | lifetime cleanup skips completed releases but preserves explicit repeated release |
 | fs | Test cleanup joins in-flight releases and releases unreleased resources after failure | new (neither copy) | lifetime cleanup joins in-flight releases and cleans unreleased locks after failure |

src/rpc/rpc-client.ts

@@ -147,10 +147,11 @@ async function request<T>(
             if (!status) return done(null)
             const code = Number(status[1])
             if (code < 200 || code >= 300)
-              return done(
-                null,
-                [401, 403, 404, 410, 502, 503, 504].includes(code),
-              )
+              // Only statuses meaning "this is not the server you meant" may
+              // rediscover and resend. A 504 is this server's apply deadline:
+              // the handler can still be running, so resending it to another
+              // server could run the command twice. 5xx is never retried.
+              return done(null, [401, 403, 404, 410].includes(code))
             for (const header of headers.slice(1)) {
               const colon = header.indexOf(':')
               const name = header.slice(0, colon).toLowerCase()

test/rpc/client-discovery-cache.test.ts

@@ -130,7 +130,7 @@ test('connect failure rediscovery succeeds in the same pending call', async () =
   })
 })
 
-for (const status of [401, 403, 404, 503]) {
+for (const status of [401, 403, 404, 410]) {
   test(`stale status ${status} rediscovery succeeds in the same pending call`, async () => {
     await fixture(async (dir, serve) => {
       const replacement = await serve()
@@ -149,6 +149,30 @@ for (const status of [401, 403, 404, 503]) {
   })
 }
 
+// An apply that hit the server's deadline may still be running there, so the
+// client must not resend it to a newer server in the same directory.
+for (const status of [500, 502, 503, 504]) {
+  test(`status ${status} is not resent to another server`, async () => {
+    await fixture(async (dir, serve) => {
+      const replacement = await serve()
+      const busy = await serve(async (socket) => {
+        await writePortFile(dir, { ...replacement.entry, token: 'new-token' })
+        socket.end(`HTTP/1.0 ${status} Busy\r\n\r\n{}`)
+      })
+      await writePortFile(dir, busy.entry)
+      const client = createRpcClient(
+        dir,
+        process.pid,
+        undefined,
+        discoveryCounter(),
+      )
+      expect(await client.pending(0)).toEqual([])
+      expect(busy.calls()).toBe(1)
+      expect(replacement.calls()).toBe(0)
+    })
+  })
+}
+
 test('cached selections never weaken exactPid refusal for a dead PID', async () => {
   await fixture(async (dir, serve) => {
     const live = await serve()