Skip to content

fix(chat): key chat history on a stable workspace identifier - #584

Open
laileni-aws wants to merge 2 commits into
feature/OHIfrom
fix/stable-chat-history-workspace-id
Open

laileni-aws wants to merge 2 commits into
feature/OHIfrom
fix/stable-chat-history-workspace-id

Conversation

@laileni-aws

@laileni-aws laileni-aws commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Problem

After an Eclipse restart (or plugin update), previously open Amazon Q chat tabs are intermittently not restored. Sometimes an old, already-closed tab is restored instead. Plugin logs are clean when this happens. Reported in #565 (split out of #559).

Root cause

The language server stores chat history in ~/.aws/amazonq/history/chat-history-<workspaceId>.json. It derives workspaceId from awsClientCapabilities.q.workspaceFilePath when the client provides one, and otherwise falls back to hashing the workspace folders sent in the LSP initialize request:

  • Multiple folders — MD5 of the folder paths sorted and joined with |.
  • One folder — MD5 of that single path, with no sorting or joining.
  • No folders — the literal string no-workspace, i.e. one shared history file.

The Eclipse plugin does not send workspaceFilePath, so the fallback is used. LSP4E populates workspaceFolders from the set of open projects, so the history file identity changes whenever a project is opened, closed, imported or deleted between sessions. On the next start the server opens a different history file: either a brand-new one (no tabs restored) or an older one whose isOpen flags are stale (an old closed tab is restored). This matches the reporter's manual workaround of renaming the history file.

Fix

Send the canonicalized Eclipse workspace root path (ResourcesPlugin.getWorkspace().getRoot().getLocation()) as awsClientCapabilities.q.workspaceFilePath. This is stable for the lifetime of a workspace regardless of which projects are open, and is the same mechanism the other IDE plugins use.

The path is canonicalized so a symlinked -data directory, a redundant path segment, or a different drive-letter case all resolve to one identifier, with a fallback to the literal path if canonicalization fails.

Migration behaviour, precisely

When the server first sees workspaceFilePath it hashes it with SHA-256 (not MD5 — that distinguishes the new scheme from older plugins that MD5-hashed the same field) and, only if no file exists at the new name, performs a one-time renameSync of the folder-based file that matches the projects open at that first launch.

Consequences worth knowing before merge:

  • Only that one file is migrated. History files for other project-set combinations stay on disk, orphaned.
  • Because it renames rather than copies, downgrading to a plugin version that does not send workspaceFilePath shows empty history.
  • If no projects are open at first launch, the old identifier is no-workspace, so the shared chat-history-no-workspace.json is moved into this workspace's file and is no longer available to other workspaces that start with no projects open.

Testing

  • mvn -pl plugin -am -Dskip.npm -Dskip.installnodenpm verify → BUILD SUCCESS, 536 tests, 0 failures, 0 errors, 0 Checkstyle violations.
  • WorkspaceUtilsTest (6): canonical path returned; redundant path segments resolve to the same id; fallback to the literal path when canonicalization fails, with the exception logged; null location; blank path short-circuits before touching the filesystem; ResourcesPlugin unavailable.
  • AmazonQLspServerBuilderTest (3, new): drives an initialize request through wrapMessageConsumer and asserts aws.awsClientCapabilities.q.workspaceFilePath is present with the workspace root, absent when the root is unknown, and that the other q capabilities are unaffected.

Not verified: the end-to-end restart behaviour in a running Eclipse instance on Linux and Windows — open two tabs on the old build, upgrade and restart to see Migrating history file… with both tabs back, then add or close a project and restart to confirm the Initializing database at path is unchanged. I do not have an Eclipse runtime available, so this still needs a manual pass before merge.

Notes

  • workspaceFilePath is read only by the chat history database on the server side; no other server feature consumes it.
  • Side benefit: two Eclipse workspaces that happen to have the same projects open no longer share a single history file.
  • Server-side follow-ups, out of scope here: the migration renameSync runs unguarded during ChatDatabase construction, so a failure there would prevent chat from starting; copying instead of renaming, and skipping the shared no-workspace file, would both be safer.

Send the Eclipse workspace root as awsClientCapabilities.q.workspaceFilePath
in the LSP initialize request. The language server uses it to name the chat
history database; without it the server falls back to hashing the set of open
project folders, which changes whenever a project is opened, closed, imported
or deleted. On the next restart a different history file is loaded, so
previously open chat tabs are not restored (or stale tabs from an old file
are restored instead).

The server migrates the existing folder-based history file to the new
identifier the first time it sees workspaceFilePath, so existing chat history
is preserved.
@laileni-aws
laileni-aws marked this pull request as ready for review October 5, 2026 19:15
@ashishrp-aws

Copy link
Copy Markdown
Contributor

🤖 AI-assisted review (6 independent reviewer agents + a chair that re-verified contested claims).

The approach looks right. I checked the server against language-servers @7c9f732. The key path you set (AmazonQLspServerBuilder.java:67-71) matches chatDb.ts:173-175, and chatDb.ts is the only consumer. Thanks for this.

Before merge

  • Run it end to end on Linux and Windows. Open 2 tabs on the old build. Upgrade and restart: you should see Migrating history file… and both tabs back. Then close or add a project and restart: the Initializing database at path should stay the same. The PR body says "Draft", but the PR isn't marked as one.

Should fix

  • Correct the PR description:
    • The new id is SHA-256 of the raw path (chatDb.ts:180, util.ts:519). The MD5 fallback sorts and joins only for multiple folders (chatDb.ts:147-166).
    • Migration is a one-time renameSync of the single file matching the projects open at first launch (chatDb.ts:204-209). Older project-set files stay orphaned. Downgrading shows empty history.
    • With no projects open, the shared chat-history-no-workspace.json is moved into this workspace (chatDb.ts:166).
  • Add a builder test (via wrapMessageConsumer with an initialize request) asserting aws.awsClientCapabilities.q.workspaceFilePath is set, and absent when null. A typo there would pass CI silently.
  • Log the exception with warn("…", e) at WorkspaceUtils.java:44.

Nits

  • Consider toFile().getCanonicalPath() with a fallback (WorkspaceUtils.java:41). A symlinked -data path or a different drive-letter case currently gives a different id.
  • getWorkspaceRootPath() would describe what the method returns better.
  • The isNotBlank check at :68 repeats WorkspaceUtils.java:42.

Server follow-ups (not this PR)

  • renameSync runs unguarded in the ChatDatabase constructor (chatDb.ts:81, agenticChatController.ts:366). A throw there would stop chat from starting.
  • Consider copying instead of renaming, and skipping 'no-workspace'.

Side benefit worth a changelog line: two Eclipse workspaces with the same projects no longer share one history file.

Review feedback:

- Canonicalize the workspace root path. A symlinked -data directory, a
  redundant path segment, or a different drive-letter case previously
  produced a different identifier, which defeats the point of a stable
  id. Falls back to the literal path if canonicalization fails. The
  blank check runs first, because new File("").getCanonicalPath()
  resolves to the process working directory.

- Add AmazonQLspServerBuilderTest, driving an initialize request through
  wrapMessageConsumer and asserting aws.awsClientCapabilities.q.
  workspaceFilePath is present with the workspace root and absent when
  the root is unknown. That key is a plain string in a nested map, so a
  typo would otherwise compile and pass CI silently.

- Log the caught exception instead of only its message.

- Rename getWorkspaceFilePath to getWorkspaceRootPath, which describes
  what the method returns rather than the protocol field it feeds.

- Drop the redundant isNotBlank check in the builder; the util already
  returns null rather than a blank string, so a null check is enough.
@laileni-aws

Copy link
Copy Markdown
Contributor Author

Thanks — I verified each of the factual points against chatDb.ts / util.ts before changing anything, and all of them hold. Pushed in 4095010.

Should fix — done

  • PR description corrected. It now states that the new id is SHA-256 of the raw path, that the MD5 fallback sorts and joins only when there is more than one folder (a single folder is hashed directly, and no folders yields the literal no-workspace), and it spells out the migration precisely: a one-time renameSync, only when no file exists at the new name, of the single folder-based file matching the projects open at that first launch. I also called out the three consequences — older project-set files stay orphaned, downgrading shows empty history because it renames rather than copies, and with no projects open the shared chat-history-no-workspace.json gets moved into this workspace and stops being available to other workspaces that start empty.
  • Builder test added. AmazonQLspServerBuilderTest drives an initialize request through wrapMessageConsumer and asserts aws.awsClientCapabilities.q.workspaceFilePath is present with the workspace root, absent when the root is unknown, and that the other q capabilities are untouched. You were right that a typo there would have passed silently.
  • Exception logged — warn("Failed to determine workspace location", e) instead of concatenating e.getMessage().

Nits — done

  • Canonicalized via toFile().getCanonicalPath() with a fallback to the literal path, and the fallback logs the IOException. Two notes: the blank check now runs before canonicalization, because new File("").getCanonicalPath() resolves to the process working directory and would be a wrong and unstable id; and since this PR has not shipped there is no deployed identifier to preserve, so canonicalizing now costs nothing. Covered by a test that asserts two spellings of one directory produce the same id.
  • Renamed to getWorkspaceRootPath().
  • Removed the duplicate isNotBlank; the util returns null rather than a blank string, so a null check is sufficient.

Before merge — partly open

mvn -pl plugin -am -Dskip.npm -Dskip.installnodenpm verify is green: BUILD SUCCESS, 536 tests, 0 failures, 0 errors, 0 Checkstyle violations.

The end-to-end restart check on Linux and Windows is still outstanding — I don't have an Eclipse runtime available, so I could not watch for Migrating history file… or confirm the Initializing database at path stays put across a project add/close. That needs a manual pass before merge and I've said so in the PR body rather than implying it was covered.

On the draft point: the body's stale "Draft" line is gone. The PR itself is not marked draft, and I've left that as-is rather than flipping its state after you'd already reviewed it.

Server follow-ups — agreed and left out of this PR. The unguarded renameSync during ChatDatabase construction is the one I'd prioritise, since a throw there stops chat from starting rather than just losing history. Copying instead of renaming, and skipping no-workspace, would also address the orphaning and downgrade behaviour above.

Added the side benefit you mentioned to the PR body: two Eclipse workspaces with the same projects open no longer share one history file. This repo has no .changes directory or changelog file, so there was nowhere to put a changelog entry — happy to add one if there's a convention I missed.

@ashishrp-aws ashishrp-aws left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🤖 AI-assisted review. Requesting changes per the blocking items in #584 (comment) — happy to re-review once they're addressed.

@laileni-aws
laileni-aws changed the base branch from main to feature/OHI October 7, 2026 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants