Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
17 changes: 17 additions & 0 deletions changelog.d/unreleased/4059.fixed.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
---
category: fixed
issues:
- 4059
affected:
- src/CodeIndex/Cli/DiffCommandRunner.cs
- src/CodeIndex/Cli/ExportImportCommandRunner.cs
- docs/dotnet-risk-audit-4059.md
---

## English

- **.NET sync/cancellation audit hits are classified and narrowed (#4059)** — unnecessary no-token helper wrappers were removed from diff and export/import internals, while the remaining intentional compatibility, process-boundary, and detached-cleanup cases are documented.

## 日本語

- **.NET の sync/cancellation 監査ヒットを分類し、対象を絞りました (#4059)** — diff と export/import 内部の不要な no-token helper wrapper を削除し、残す互換 API、process 境界、detached cleanup のケースを文書化しました。
95 changes: 95 additions & 0 deletions docs/dotnet-risk-audit-4059.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
# .NET Sync/Cancellation Risk Audit (#4059)

> [日本語版はこちら / Japanese version](#net-synccancellation-risk-audit-4059-日本語)

This note classifies the production `dotnet-risk-patterns/cancellation-token-none`
and `dotnet-risk-patterns/sync-over-async` hits reviewed for #4059.

## English

### Remediated hits

| Area | Prior hit | Classification | Action |
|---|---|---|---|
| `DiffCommandRunner` | Private `DiffOrderedRows` and `RowsEqual` overloads without a token | Fix-needed convenience wrappers | Removed. Production callers already had a caller token. |
| `ExportImportCommandRunner` | Internal no-token helpers for archive writes, table counts, database snapshots, and import replacement | Fix-needed/test convenience wrappers | Removed. Tests now call token-aware helpers with an explicit `CancellationToken.None`. |

### Retained `CancellationToken.None` hits

| Area | Classification | Rationale |
|---|---|---|
| `DbWriter` transaction and batch-write convenience overloads | Intentional compatibility API | Token-aware overloads exist and indexing paths use them; the no-token overloads preserve existing direct callers. |
| `DbContext` checkpoint and vacuum convenience overloads | Intentional compatibility API | Token-aware overloads exist for command paths; the no-token overloads remain direct-call conveniences. |
| `DbCommandRunner.Run`, `DiffCommandRunner.Run`, and `QueryCommandRunner.RunVacuum` | CLI compatibility boundary | `ProgramRunner` dispatches with its cancellation token; the no-token overloads remain for direct callers/tests. |
| `LspServer.Run` and `LspServer.TryReadMessage` | Legacy synchronous wrapper | Async/token-aware entrypoints are the preferred transport path and are documented next to the wrappers. |
| `McpServer` request-token resets and late continuations | Detached cleanup/observer boundary | These continuations must still run after request cancellation to release gates, dispose linked tokens, or observe late faults. |
| `HttpMcpTransport` late output observer and response fallback | Detached observer or no-request fallback | Abandoned output writes need an uncancelled fault observer; fallback responses have no `PendingRequest` token. |
| `IndexCommandRunner.FullScan` and `IndexCommandRunner.UpdateTargets` continuations | Detached cleanup boundary | Completion/heartbeat cleanup must run even when the caller token has been canceled. |
| `GitHelper.WaitForGitExitAsync` timeout delay | Timeout sentinel | The timeout task must not be canceled by caller cancellation because cancellation is represented by a separate task. |
| `IssueDuplicatePreflight.TryLoad` | Legacy synchronous wrapper | Async/token-aware loading is used by command paths that have caller cancellation. |
| `SuggestionStore.TryAddAndSubmit` and no-token async convenience overload | Compatibility API | MCP uses the token-aware async overload; the existing sync/direct async overloads remain for direct callers. |
| `DbReader` legacy constructor | Compatibility API | MCP and other request-scoped paths use the token-aware constructor; the old constructor preserves direct callers. |
| `AuditLogSink.WaitForIdle` and `BackgroundTaskObserver.Run(Func<Task>, ...)` | Convenience overload | Token-aware overloads exist for callers that own a meaningful cancellation token. |

### Retained `sync-over-async` hits

| Area | Classification | Rationale |
|---|---|---|
| `ProgramRunner` MCP and HTTP MCP launch waits | Process/CLI boundary | The top-level command runner is synchronous; async server loops still observe signal/request cancellation internally. |
| `ProgramRunner.RunInstallerProcessDetailed` | Process/CLI boundary | The method is bounded by timeout/cancellation and kills the subprocess on cancellation or timeout. |
| `McpServer.ProcessFrame` and `McpServer.HandleMessage` | Legacy synchronous wrapper | Transports use async entrypoints; wrappers remain for legacy in-process callers and tests. |
| `McpServer.ObserveCompletedRequestTask` | Completed-task fault observer | It only observes an already-faulted task so the exception is logged without changing shutdown behavior. |
| `LspServer.Run` and `LspServer.TryReadMessage` | Legacy synchronous wrapper | Async/token-aware APIs are available for transports with shutdown tokens. |
| `GitHelper.RunProcessCapturingResult` | Synchronous git helper boundary | Callers propagate cancellation; converting the helper graph to async is broader than #4059 and not required for shutdown correctness. |
| `SymbolExtractionWorker` and `PostExtractionHookCallbackWorker` | Isolated worker protocol boundary | `WaitForTask` proves send/read tasks completed, timed out, or observed cancellation before `GetResult` reads the completed result. |
| `ConsoleUi.StopSpinner` | Synchronous teardown boundary | Spinner shutdown is best-effort during a synchronous dispose-style path; faults are intentionally swallowed after observation. |
| `IssueDuplicatePreflight.TryLoad` and `SuggestionStore.TryAddAndSubmit` | Legacy synchronous wrapper | Command/MCP paths with caller cancellation use async/token-aware APIs. |

After the remediation above, no remaining #4059 audit hit is classified as fix-needed.

<a id="net-synccancellation-risk-audit-4059-日本語"></a>
## .NET sync/cancellation risk audit (#4059) 日本語

このメモは #4059 で確認した production の
`dotnet-risk-patterns/cancellation-token-none` と
`dotnet-risk-patterns/sync-over-async` のヒットを分類する。

### 対応したヒット

| 領域 | 以前のヒット | 分類 | 対応 |
|---|---|---|---|
| `DiffCommandRunner` | token を受け取らない private `DiffOrderedRows` / `RowsEqual` overload | 修正対象の convenience wrapper | 削除。production 呼び出し側は既に caller token を持っていた。 |
| `ExportImportCommandRunner` | archive 書き込み、table count、DB snapshot、import replacement の internal no-token helper | 修正対象 / test convenience wrapper | 削除。テストは token-aware helper に明示的な `CancellationToken.None` を渡す。 |

### 残す `CancellationToken.None` ヒット

| 領域 | 分類 | 理由 |
|---|---|---|
| `DbWriter` の transaction / batch write convenience overload | 意図的な互換 API | token-aware overload は存在し、indexing 経路はそちらを使う。no-token overload は既存 direct caller の互換用に残す。 |
| `DbContext` の checkpoint / vacuum convenience overload | 意図的な互換 API | command 経路用の token-aware overload は存在する。no-token overload は direct call 用の便宜 API。 |
| `DbCommandRunner.Run`、`DiffCommandRunner.Run`、`QueryCommandRunner.RunVacuum` | CLI 互換境界 | `ProgramRunner` dispatch は cancellation token を渡す。no-token overload は direct caller / test 用に残す。 |
| `LspServer.Run` と `LspServer.TryReadMessage` | legacy synchronous wrapper | transport では async / token-aware entrypoint を優先し、wrapper 近傍にもその前提を記載済み。 |
| `McpServer` の request-token reset と late continuation | detached cleanup / observer 境界 | request cancellation 後でも gate 解放、linked token dispose、late fault 観測を実行する必要がある。 |
| `HttpMcpTransport` の late output observer と response fallback | detached observer または no-request fallback | 放棄された出力書き込みには未キャンセルの fault observer が必要で、fallback response には `PendingRequest` token がない。 |
| `IndexCommandRunner.FullScan` と `IndexCommandRunner.UpdateTargets` の continuation | detached cleanup 境界 | caller token がキャンセル済みでも completion / heartbeat cleanup は実行する必要がある。 |
| `GitHelper.WaitForGitExitAsync` の timeout delay | timeout sentinel | timeout task は caller cancellation でキャンセルしてはいけない。キャンセルは別 task で表す。 |
| `IssueDuplicatePreflight.TryLoad` | legacy synchronous wrapper | caller cancellation を持つ command 経路は async / token-aware loading を使う。 |
| `SuggestionStore.TryAddAndSubmit` と no-token async convenience overload | 互換 API | MCP は token-aware async overload を使う。既存の sync / direct async overload は direct caller 用に残す。 |
| `DbReader` の legacy constructor | 互換 API | MCP など request-scoped 経路は token-aware constructor を使う。旧 constructor は direct caller 互換用。 |
| `AuditLogSink.WaitForIdle` と `BackgroundTaskObserver.Run(Func<Task>, ...)` | convenience overload | 意味のある cancellation token を持つ caller 向けの token-aware overload が存在する。 |

### 残す `sync-over-async` ヒット

| 領域 | 分類 | 理由 |
|---|---|---|
| `ProgramRunner` の MCP / HTTP MCP 起動待ち | process / CLI 境界 | top-level command runner は同期 API。async server loop 自体は signal / request cancellation を内部で観測する。 |
| `ProgramRunner.RunInstallerProcessDetailed` | process / CLI 境界 | timeout / cancellation で境界があり、キャンセルまたは timeout 時には subprocess を kill する。 |
| `McpServer.ProcessFrame` と `McpServer.HandleMessage` | legacy synchronous wrapper | transport は async entrypoint を使う。wrapper は legacy in-process caller と test 用。 |
| `McpServer.ObserveCompletedRequestTask` | completed-task fault observer | 既に fault した task を観測してログへ流すだけで、shutdown 挙動は変えない。 |
| `LspServer.Run` と `LspServer.TryReadMessage` | legacy synchronous wrapper | shutdown token を持つ transport 向けに async / token-aware API がある。 |
| `GitHelper.RunProcessCapturingResult` | 同期 git helper 境界 | caller は cancellation を伝搬する。helper graph 全体の async 化は #4059 より広く、shutdown correctness には不要。 |
| `SymbolExtractionWorker` と `PostExtractionHookCallbackWorker` | isolated worker protocol 境界 | `WaitForTask` が send/read task の完了、timeout、または cancellation 観測を確認してから、完了済み結果を `GetResult` で読む。 |
| `ConsoleUi.StopSpinner` | 同期 teardown 境界 | spinner shutdown は同期 dispose 風の経路で best-effort。fault は観測後に意図的に握りつぶす。 |
| `IssueDuplicatePreflight.TryLoad` と `SuggestionStore.TryAddAndSubmit` | legacy synchronous wrapper | caller cancellation を持つ command / MCP 経路は async / token-aware API を使う。 |

上記対応後、#4059 の残存 audit hit に修正対象として分類されるものはない。
6 changes: 0 additions & 6 deletions src/CodeIndex/Cli/DiffCommandRunner.cs
Original file line number Diff line number Diff line change
Expand Up @@ -412,9 +412,6 @@ private static DiffJsonResult BuildSchemaMismatchDiff(DiffDbHeader left, DiffDbH
options.Detailed);
}

private static OrderedRowsDiff DiffOrderedRows(SqliteConnection leftConnection, SqliteConnection rightConnection, string sql, int limit)
=> DiffOrderedRows(leftConnection, rightConnection, sql, sql, limit, CancellationToken.None);

private static OrderedRowsDiff DiffOrderedRows(
SqliteConnection leftConnection,
SqliteConnection rightConnection,
Expand Down Expand Up @@ -553,9 +550,6 @@ private static OrderedRowsDiff DiffOrderedStrings(SqliteConnection leftConnectio
return new OrderedRowsDiff(equal, onlyInLeft, onlyInRight, truncated);
}

private static bool RowsEqual(SqliteConnection leftConnection, SqliteConnection rightConnection, string sql)
=> RowsEqual(leftConnection, rightConnection, sql, sql, CancellationToken.None);

private static bool RowsEqual(SqliteConnection leftConnection, SqliteConnection rightConnection, string sql, CancellationToken cancellationToken)
=> RowsEqual(leftConnection, rightConnection, sql, sql, cancellationToken);

Expand Down
12 changes: 0 additions & 12 deletions src/CodeIndex/Cli/ExportImportCommandRunner.cs
Original file line number Diff line number Diff line change
Expand Up @@ -677,9 +677,6 @@ private static void AddTextEntry(ZipArchive archive, string name, string content
writer.Write(content);
}

internal static void WriteExportArchiveFile(string outputPath, string snapshotPath, ExportManifest manifest, JsonSerializerOptions jsonOptions)
=> WriteExportArchiveFile(outputPath, snapshotPath, manifest, jsonOptions, CancellationToken.None);

internal static void WriteExportArchiveFile(string outputPath, string snapshotPath, ExportManifest manifest, JsonSerializerOptions jsonOptions, CancellationToken cancellationToken)
{
var fullOutputPath = Path.GetFullPath(outputPath);
Expand Down Expand Up @@ -1103,9 +1100,6 @@ private static bool ValidateNonNegativeManifestInt(int? value, string fieldName,
return true;
}

private static long ReadTableCount(SqliteConnection connection, string tableName)
=> ReadTableCount(connection, tableName, CancellationToken.None);

private static long ReadTableCount(SqliteConnection connection, string tableName, CancellationToken cancellationToken)
{
cancellationToken.ThrowIfCancellationRequested();
Expand Down Expand Up @@ -1398,9 +1392,6 @@ private static string FormatImportSuccessMessage(string prefix, bool prunePaths,
? $"{prefix}; pruned paths to project root {importTargetProjectRoot}"
: prefix;

internal static void CreateDatabaseSnapshot(string sourceDbPath, string snapshotPath)
=> CreateDatabaseSnapshot(sourceDbPath, snapshotPath, CancellationToken.None);

internal static void CreateDatabaseSnapshot(string sourceDbPath, string snapshotPath, CancellationToken cancellationToken)
{
cancellationToken.ThrowIfCancellationRequested();
Expand All @@ -1421,9 +1412,6 @@ private static string CreateUnpooledConnectionString(string dbPath)
private static string CreateReadOnlyUnpooledConnectionString(string dbPath)
=> SqliteConnectionPolicy.BuildConnectionString(dbPath, SqliteConnectionPolicyMode.ReadOnlyUnpooled);

internal static void ReplaceImportedDatabase(string tempPath, string fullDbPath)
=> ReplaceImportedDatabase(tempPath, fullDbPath, CancellationToken.None);

internal static void ReplaceImportedDatabase(string tempPath, string fullDbPath, CancellationToken cancellationToken)
{
cancellationToken.ThrowIfCancellationRequested();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ public void ReplaceImportedDatabase_PostMoveCancellationRollsBackDestination_Iss
throw new OperationCanceledException();

Assert.Throws<OperationCanceledException>(() =>
ExportImportCommandRunner.ReplaceImportedDatabase(tempPath, dbPath));
ExportImportCommandRunner.ReplaceImportedDatabase(tempPath, dbPath, CancellationToken.None));

Assert.Equal("existing db", File.ReadAllText(dbPath));
Assert.False(File.Exists(tempPath));
Expand Down
15 changes: 8 additions & 7 deletions tests/CodeIndex.Tests/ExportImportCommandRunnerTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -861,7 +861,7 @@ public void CreateDatabaseSnapshot_AppliesPrivateFileMode()
var sourceDbPath = TestProjectHelper.CreateProjectDb(projectRoot);
var snapshotPath = Path.Combine(projectRoot, "snapshot.db");

ExportImportCommandRunner.CreateDatabaseSnapshot(sourceDbPath, snapshotPath);
ExportImportCommandRunner.CreateDatabaseSnapshot(sourceDbPath, snapshotPath, CancellationToken.None);

var mode = File.GetUnixFileMode(snapshotPath) & DataDirectorySecurity.PermissionBits;
Assert.Equal(DataDirectorySecurity.PrivateFileMode, mode);
Expand Down Expand Up @@ -920,7 +920,8 @@ public void WriteExportArchiveFile_FailurePreservesExistingArchive()
outputPath,
missingSnapshotPath,
manifest,
new JsonSerializerOptions()));
new JsonSerializerOptions(),
CancellationToken.None));

Assert.Equal("existing archive", File.ReadAllText(outputPath));
Assert.Single(Directory.GetFiles(workDir));
Expand Down Expand Up @@ -1002,7 +1003,7 @@ public void ReplaceImportedDatabase_MoveFailurePreservesExistingSidecars()
var missingTempPath = Path.Combine(workDir, "missing.db");

Assert.ThrowsAny<IOException>(() =>
ExportImportCommandRunner.ReplaceImportedDatabase(missingTempPath, dbPath));
ExportImportCommandRunner.ReplaceImportedDatabase(missingTempPath, dbPath, CancellationToken.None));

Assert.Equal("existing db", File.ReadAllText(dbPath));
Assert.Equal("existing wal", File.ReadAllText(dbPath + "-wal"));
Expand All @@ -1028,7 +1029,7 @@ public void ReplaceImportedDatabase_SuccessDeletesDestinationSidecarsAfterMove()
File.WriteAllText(dbPath + "-shm", "existing shm");
File.WriteAllText(tempPath, "imported db");

ExportImportCommandRunner.ReplaceImportedDatabase(tempPath, dbPath);
ExportImportCommandRunner.ReplaceImportedDatabase(tempPath, dbPath, CancellationToken.None);

Assert.Equal("imported db", File.ReadAllText(dbPath));
Assert.False(File.Exists(dbPath + "-wal"));
Expand Down Expand Up @@ -1058,7 +1059,7 @@ public void ReplaceImportedDatabase_SidecarBackupCleanupFailureLeavesReplacement

var (_, _, stderr) = ConsoleCapture.Capture(() =>
{
ExportImportCommandRunner.ReplaceImportedDatabase(tempPath, dbPath);
ExportImportCommandRunner.ReplaceImportedDatabase(tempPath, dbPath, CancellationToken.None);
return 0;
});

Expand Down Expand Up @@ -1092,7 +1093,7 @@ public void ReplaceImportedDatabase_PostMoveFailureRollsBackDestination_Issue341
throw new IOException("simulated post-move failure");

var ex = Assert.ThrowsAny<IOException>(() =>
ExportImportCommandRunner.ReplaceImportedDatabase(tempPath, dbPath));
ExportImportCommandRunner.ReplaceImportedDatabase(tempPath, dbPath, CancellationToken.None));

Assert.Contains("rolled back", ex.Message, StringComparison.Ordinal);
Assert.Equal("existing db", File.ReadAllText(dbPath));
Expand Down Expand Up @@ -1123,7 +1124,7 @@ public void ReplaceImportedDatabase_AppliesPrivateFileMode()
File.WriteAllText(tempPath, "imported db");
File.SetUnixFileMode(tempPath, DataDirectorySecurity.PermissionBits);

ExportImportCommandRunner.ReplaceImportedDatabase(tempPath, dbPath);
ExportImportCommandRunner.ReplaceImportedDatabase(tempPath, dbPath, CancellationToken.None);

var mode = File.GetUnixFileMode(dbPath) & DataDirectorySecurity.PermissionBits;
Assert.Equal(DataDirectorySecurity.PrivateFileMode, mode);
Expand Down
Loading